-
Notifications
You must be signed in to change notification settings - Fork 765
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
@pypi and faster @conda #1505
@pypi and faster @conda #1505
Conversation
8508764
to
cd78065
Compare
All test failures are due to a (un)related issue with how we vendor packages. |
de5cdfa
to
7ed6ebf
Compare
butterfingered approval, will still run through some tests but changes I requested are ok :)
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.
expected functionality seems to be in place. tested both conda and pypi decorator usage with sfn,batch,argo, kubernetes, and local execution. No issues found so approving from my end :)
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.
Not done. Pushing current comments as requested.
# Create Conda environment. | ||
cmds = [ | ||
# TODO: check if mamba or conda are already available on the image | ||
f"""if ! command -v ./micromamba >/dev/null 2>&1; then |
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.
why just in current dir?
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.
OK. I think I finished looking at it. It's mostly minor stuff. I think the pinned dep stuff warrants fixing since I don't see a compelling reason to change it (unless I am missing something). It feels gratuitous to break something that people may be relying on (as you say, if it's in the code :) ).
There is some concern around the general way things are done in a not very multiple-process safe kind of way (even removing some of the previous locks that were in the previous implementation) so I suspect that we will hit some issues there at some point but it probably won't impact users directly as it is not very common.
#1567 re-enables pinned_conda_libs. re - removing the locks - it shouldn't be an issue anymore. Dangling lock files were a big headache in the previous implementation - now everything happens in a multi-process safe manner - so we should be good. |
To try it out