-
Notifications
You must be signed in to change notification settings - Fork 18
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
ENH: Added Python version to vetiver_pin_write
#127
Conversation
Welcome Ganesh! Thank you so much for this contribution, and it was great to meet you at PyData NYC. :) I think |
Ah that makes sense, thanks for the info! Will keep an eye out for that PR and rebase when done. Will keep looking around other stuff in the project meanwhile 👍 |
Hi Ganesh-- the other PR (#126) has been merged! It should be easier to add the Python version to the metadata dataclass now :) |
Thanks! Sorry was not active, had a few ideas but got caught up. I'll try to finish it this week. |
b40b543
to
3dd7690
Compare
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.
One comment on sys.version
vs. sys.version_info
, but this looks so great 💯 I appreciate you getting your hands on the new metadata structure!
vetiver/meta.py
Outdated
@@ -25,9 +27,10 @@ def from_dict(cls, metadata, pip_name=None) -> "VetiverMeta": | |||
version = metadata.get("version", None) | |||
url = metadata.get("url", None) | |||
required_pkgs = metadata.get("required_pkgs", []) | |||
python_version = metadata.get("python_version", sys.version) |
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.
python_version = metadata.get("python_version", sys.version) | |
python_version = metadata.get("python_version", sys.version_info) |
Later on, I think we want to be able to quickly compare the version of Python that the model was originally written with the version of Python that the user has locally (if model is read in from some other location). Using version_info
for a tuple rather than a string will probably be more straightforward for this comparison.
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.
Thanks! That makes sense. I had pickling issues with tuples
(yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/tuple'
), so I'm kinda handling it in a not-so-favorable way here: pin_read_write.py. I guess there are no issues as the end user will still deal with tuples, but please do let me know if a cleaner approach pops up.
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.
Ah, I see that! I don't believe there is a cleaner approach, but will keep that in mind 👀
3dd7690
to
c049053
Compare
c049053
to
9325d35
Compare
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.
Looks great! Thank you so much @ganesh-k13 🙌
Changes
This adds the python system information to the metadata
Note
Hey folks! I found your repo through PyData NYC and was interested in contributing mainly to the deployment aspect. This is a basic PR for one of the issues to get a hang of the project.
Testing:
There is no UT :)It's inside another folder, my badSample run:
Alternatively
We could add it to _model_meta to make it global? Seemed too invasive so wanted a second opinion
resolves: #115