-
Notifications
You must be signed in to change notification settings - Fork 94
disable jax if PYMBAR_DISABLE_JAX is set #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
disable jax if PYMBAR_DISABLE_JAX is set #569
Conversation
pymbar/tests/test_mbar_solvers.py
Outdated
def test_env_values(self, monkeypatch, env_value, expected): | ||
"""Test various environment variable values.""" | ||
monkeypatch.setenv("PYMBAR_DISABLE_JAX", env_value) | ||
assert _setup_jax_accleration() is expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to specify an error message in case the assertion fails? Something like "received X but expected Y"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest will do that for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, I think this works for our purposes. Just some minor changes/comments.
In the long term I wonder if we want to make the MBAR
object itself specify the backend via an argument.
Co-authored-by: Iván Pulido <[email protected]>
Co-authored-by: Iván Pulido <[email protected]>
…x-at-runtime-option' into feat/add-disable-jax-at-runtime-option
#509 is how we will fix this long term, but this should work for now. |
pymbar/tests/test_mbar_solvers.py
Outdated
("yes", True), | ||
("YES", True), | ||
("YeS", True), | ||
("0", True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect 0=False and 1=True, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duh! I was thinking about 0
being the "good" exit code and that was "true" to be BUT as soon as you mention that, it is the opposite of the convention
Co-authored-by: Alyssa Travitz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. One minor comment, but otherwise LGTM.
README.md
Outdated
|
||
However, many packages released on conda-forge specify `pymbar` as a dependency, making it impossible to install the non-jax accelerated of PyMbar. | ||
In `pymbar >= 4.2.0` JAX acceleration can be controlled with the environmental variable `PYMBAR_DISABLE_JAX`. | ||
Setting `PYMBAR_DISABLE_JAX` to a truth-y value (`TRUE`, `YES`, `0`) will disable JAX acceleration even if the `jax` package is installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we mean here 1
instead of 0
as a truth-y value?
…x-at-runtime-option' into feat/add-disable-jax-at-runtime-option
This will let a user disable jax at runtime. A larger refactor is needed to test this since at import time the
PYMBAR_DISABLE_JAX
variable is checked, so even if monkey patched the module has already been imported. This also means that once pymbar has been imported, settingPYMBAR_DISABLE_JAX
will do nothing.TODO: add docs