add validation to reqs arg in cluster install pkgs method #1815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BelSasha
force-pushed
the
sb/fix-cluster-install-pkg-arg-validation
branch
from
665c7eb to
54c1d7b
Compare
| @@ -832,6 +832,8 @@ def install_packages( | |||
| >>> cluster.pip_install(reqs=["accelerate", "diffusers"]) | |||
| >>> cluster.pip_install(reqs=["accelerate", "diffusers"], conda_env_name="my_conda_env", override_remote_version=True) | |||
| """ | |||
| reqs = [reqs] if isinstance(reqs, str) else reqs | |||
There was a problem hiding this comment.
could we update the type checking in the function arg too? Union[str, List[str]]
There was a problem hiding this comment.
Also added support for passing a single rh.Package instance (and not a list of rh.Package instances).
BelSasha
force-pushed
the
sb/fix-cluster-install-pkg-arg-validation
branch
from
54c1d7b to
61ed5f5
Compare
BelSasha
force-pushed
the
sb/fix-cluster-install-pkg-arg-validation
branch
from
61ed5f5 to
5f01b75
Compare
There was a problem hiding this comment.
actually do you think we could hold off on this change? there's a couple things
- with the addition of
pip_install,conda_install, etc, it may make sense to eventually turninstall_packagesinternal facing, in which case we might not need to update the signature - on that note, if we want to support single packages it probably makes more sense to do it inside those more specific functions, which can be more vague (vs
install_packagesdoes indicate plural) - I don't think we need to support
Packageinput type for the individual functions, those should be abstracted away internally and push users towards just using strings rather than constructing packages. - and given the new direction, maybe we'll end up adjusting these APIs too so I think we can keep this in mind but no need to implement it now?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.