Skip to content
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

Python: Download bridgestan source if needed #80

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

WardBrian
Copy link
Collaborator

@WardBrian WardBrian commented Mar 8, 2023

Would close #79

This is marked as a draft due to the following requirements not yet being met:

@WardBrian
Copy link
Collaborator Author

I'd love to get some opinions on this @bob-carpenter @roualdes

@roualdes
Copy link
Owner

Re location: ~/.bridgestan/bridgestan-VERSION. Does it make sense to use the major version here, instead of version (MAJOR.MINOR.PATCH)? By using 1.0.1, couldn't a user end up many copies of effectively backwards compatible copies of bridgestan?

Re "should Julia also look in this location". I guess I'll vote no, even though on first thought it does sound somewhat appealing. Julia sharing Python's location would effectively build in an asymmetry across Python and Julia. Julia would be able to look in both artifact"bridgestan" and ~/.bridgestan/bridgestan-*, but Python would not easily be able to use Julia's location. And that seems odd to me.

@WardBrian
Copy link
Collaborator Author

Re: minor versions

Yes, if a user was really good about following updates to bridgestan they’d eventually end up with a 1.0.2, 1.0.3, … etc. I think this is actually a good thing with our versioning scheme, since then someone saying “I’m using version x.y.z” is completely unambiguous.
The Julia artifact behavior should be the same, I believe, since each version has its own hash.

The only downside I see is the potential for bloat of that directory over time. If we wanted to we could print out a message if older versions exist telling the user they’re free to delete those older ones? I don’t want to delete them automatically, since it’s possible a user could have several different versions installed e.g. in different virtual environments

@WardBrian WardBrian marked this pull request as ready for review March 16, 2023 13:47
@WardBrian WardBrian requested a review from roualdes March 16, 2023 13:47
Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

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

The immediate verification of the BridgeStan path after a user tries to set it is nice.

I'm waffling about a friendly reminder that's printed out whenever get_bridgestan_src() is called, reminding the user about all the versions of BridgeStan that exist in ~/.bridgestan. On the one hand, I see such a message as helpful. On the other hand, I don't know how to do a similar thing in Julia. So I guess we should put this off for now, and move forward with the nice feature contained in this PR.

This looks good. Thanks.

@@ -1,4 +1,5 @@
from .compile import compile_model, set_bridgestan_path
from .model import StanModel
from .__version import __version__
Copy link
Owner

Choose a reason for hiding this comment

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

Should we export __version__?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you asking if we should be doing more, or less, in the init file? Having bridgestan.__version__ defined is pretty common (e.g. https://github.com/numpy/numpy/blob/28bce82c82ba1151baeb7399abf454418b33eefe/numpy/__init__.py#L440) but I don't personally like adding it to __all__ because I don't think from bridgestan import * should import __version__ into global scope

Copy link
Owner

Choose a reason for hiding this comment

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

I was asking if we should add __version__ to __all__, with the thought (from memory) that numpy does this. But maybe my memory is incorrect here.

Your opinion makes sense to me.

And a user could still from bridgestan import __version__ correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, a user can still manually import it or access it qualified (import bridgestan; bridgestan.__version__)

It seems the "big" packages are inconsistent with whether or not they put it in __all__. numpy and scipy do, pandas and matplotlib do not. I can't find anything "official" which states a preference.

@WardBrian WardBrian merged commit 144356b into main Mar 16, 2023
@WardBrian WardBrian deleted the python/download-bridgestan branch March 16, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could "pip install bridgestan" install stan?
2 participants