FEA update_env implementation #136
Conversation
|
mypy is complaining even though I'm just passing positional arguments 🥲 |
There was a problem hiding this comment.
Looks good to me, thanks for taking this. I made two minor comments, but feel free to ignore them.
Regarding the mypy error, it sounds like they're working on a fix. Let's maybe just wait for the fix? I don't believe it's worth it to take extra measures here for an error that's (hopefully) going away very quickly.
skops/hub_utils/_hf_hub.py
Outdated
| config["sklearn"]["environment"] = requirements | ||
|
|
||
| with open(Path(path) / "config.json", mode="w") as f: | ||
| json.dump(config, f, sort_keys=True, indent=4) |
There was a problem hiding this comment.
Just throwing an idea out there: Do we want to factor this line out into a small utility function, since it's used twice now (other being _create_config)?
| @@ -402,7 +402,14 @@ def update_env( | |||
| ------- | |||
| None | |||
| """ | |||
| pass | |||
|
|
|||
There was a problem hiding this comment.
Also, can we remove the "Returns" section from the docstring, since nothing is returned?
|
Is there an open issue on mypy side? If not, we can just add type: ignore there. |
|
@adrinjalali there is: |
Note that pasting the link did create a reference in that issue ^^ I think they changed something on GH to enable this "feature". |
|
I wonder why two tests are cancelled without any tests failing, I'll trigger CI again :D |
Closes #133
This one replaces existing requirements with the new one user gives. Let me know if you'd rather have something that adds new dependencies on top of old requirements instead (or simply changes versions).