-
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: pass recursive args to checkout #6458
Conversation
@@ -348,6 +348,7 @@ def filter_pipeline(stages): | |||
quiet=True, | |||
allow_missing=True, | |||
checkpoint_reset=checkpoint_reset, | |||
recursive=kwargs.get("recursive", False), |
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.
@pmrowla, Is it not possible to checkout as we go run
-ing? This way, it may miss a lot of things. foreach
based stages come to mind, as only repro
supports them. Also, glob is not supported by checkout.
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.
Yeah we could do that. I think we'd just need to pass a flag into repro (and subsequently stage.run
) that indicates we need to checkout(allow_missing=True)
dependencies prior to reproducing a stage, right?
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.
The one thing I'm not sure on is how this affects the run cache lookup - do deps need to be checked out before that lookup or can we just skip the dep checkout entirely in the event we have a cached stage?
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.
Run-cache lookup does get affected, as if there's no dependency, there's no cached stage. This will work for workspace based run, but we may need to checkout dependencies into the tmpdir repo for the other one.
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.
LGTM, it's probably worth adding the glob issue to the next sprint to address this properly
Closes #6456