-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
exp run: support naive remote execution via --machine
#7173
Conversation
dad6dff
to
e6de537
Compare
e6de537
to
9021369
Compare
This comment has been minimized.
This comment has been minimized.
9021369
to
26f4aa4
Compare
This comment has been minimized.
This comment has been minimized.
ee6f6fe
to
d44a4f5
Compare
This pull request introduces 2 alerts and fixes 1 when merging d44a4f56adf97c7632bc251a2317c52a367d18e2 into 04e93da - view on LGTM.com new alerts:
fixed alerts:
|
19ed112
to
3aadd6f
Compare
- prerequisite for remote execution - allow explicitly pushing imports which would normally be skipped (internal-use only, does not apply to `dvc push`)
- log startup script finished message - add setup_script option for runtime env setup
3aadd6f
to
2fd4bcb
Compare
--machine
--machine
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.
So far LGTM, though I would like to read it one more time tomorrow.
As of now, I have questions more regarding usage, not necessary to be addressed in this PR:
-
Waiting for startup script to complete:
Maybe we could provide some "loading circle" placeholder? -
For the example: we had to modify the cmd to use
python3.9 {rest of command}
right?
The ubuntu machine defaults to 2.7. I wonder how to handle it, but probably the best thing is to leave it to the user. But we should probably consider changing default script to create virtualenv and activate it (if its possible). -
In
setup.sh
you only hadpip install -r requirements
, right?
Yeah eventually we could have some kind of spinner in the UI, but preferably this should really be handled on the CML side (the issue is that the CML/terraform resource works does not actually wait for the boot/startup script to finish on
This is also just the way the default CML image works (it uses ubuntu 18.04). It's possible for the user to specify alternate AWS images that would use a newer base operating system (that come with newer pythons).
The setup script creates a virtualenv and activates it first
|
2fd4bcb
to
e94900c
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.
Post-merge LGTM.
Great to see this actually working! I tried it out, but I hit some issues:
|
The explicit If the env setup script includes
Not sure what happened here, I'll have to look into it |
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π
Related to #6267
dvc exp run --machine
for running an experiment on the specifieddvc machine
instancesetup_script
config option for runtime environment setup (this is separate from the existingstartup_script
which installs system packages at machine boot/startup).Note: Until the next release, in order to actually use
dvc exp run --machine
you will need to override the defaultstartup_script
to install DVC from source with your own script:Once this PR is merged you can drop the specific
refs/pull/...
from the pip install command. Also note that you have to install some python yourself, the system python in the default CML machine image is python 3.6 which is no longer supported in DVC.