Skip to content

Conversation

@mikemhenry
Copy link
Contributor

Checklist

  • All new code is appropriately documented (user-facing code must have complete docstrings).
  • Added a news entry, or the changes are not user-facing.
  • Ran pre-commit by making a comment with pre-commit.ci autofix before requesting review.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

Developers certificate of origin

@mikemhenry mikemhenry marked this pull request as draft November 24, 2025 18:26
@mikemhenry
Copy link
Contributor Author

TODO: This needs a news item!

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.16%. Comparing base (78d0f90) to head (f290ebf).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
- Coverage   95.47%   93.16%   -2.32%     
==========================================
  Files         187      187              
  Lines       16244    16244              
==========================================
- Hits        15509    15133     -376     
- Misses        735     1111     +376     
Flag Coverage Δ
fast-tests 93.16% <100.00%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mikemhenry
Copy link
Contributor Author

We decided to add a warning that links to a document page that describes this behavior.
If we have time, we will investigate a solution that re-sets the envar to what it was before we changed it/set it.

@mikemhenry
Copy link
Contributor Author

Actually I forgot that when you set an envar within python it only exists for the python process and doesn't stay around:
$ echo "$PYMBAR_DISABLE_JAX"; python -c "import os; os.environ.setdefault('PYMBAR_DISABLE_JAX', 'TRUE')"; echo "$PYMBAR_DISABLE_JAX"

this prints 2 blank lines for me, if it stayed around, it would print a blank line and a line that says TRUE

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL Python is a shell with its own local env variables. Thanks @mikemhenry !

@mikemhenry
Copy link
Contributor Author

image to save you a click (also TIL our theme makes github URLs look nice! I just used a raw URL)

@mikemhenry
Copy link
Contributor Author

pre-commit.ci autofix

@mikemhenry mikemhenry marked this pull request as ready for review December 2, 2025 16:36
@mikemhenry mikemhenry requested a review from IAlibay December 2, 2025 16:37
Copy link
Contributor

@atravitz atravitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with adding an example error output, otherwise LGTM!

@mikemhenry
Copy link
Contributor Author

Good idea, will make searching the docs more useful -- let me know if this looks good @IAlibay @atravitz

@IAlibay IAlibay enabled auto-merge (squash) December 2, 2025 20:18
@mikemhenry mikemhenry disabled auto-merge December 2, 2025 20:28
@mikemhenry
Copy link
Contributor Author

actually I need to add a news entry, I will edit the JAX warning one

@mikemhenry mikemhenry enabled auto-merge (squash) December 2, 2025 20:31
@github-actions
Copy link

github-actions bot commented Dec 2, 2025

No API break detected ✅

@mikemhenry mikemhenry merged commit f491184 into main Dec 2, 2025
13 checks passed
@mikemhenry mikemhenry deleted the feat/disable-jax branch December 2, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants