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.
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
API/YAML: Support using env vars to define file_mounts. #2146
API/YAML: Support using env vars to define file_mounts. #2146
Changes from 3 commits
174f26e
1a3d0a4
db85fe6
2a47cf9
8aaafcd
eae0c0d
18e4177
2e259d4
460b07b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We may need to add a check for the key of the
task.envs
somewhere in our code, i.e. we need to make sure thekey ~ '[a-zA-Z][a-zA-Z0-9]*'
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.
This will get checked by storage.py's validate_name().
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.
Ahh, I was trying to say that if the user provides a envvar like the following:
Here the run section will fail, as it is not a valid syntax in bash. However, if this env var is used in the
file_mounts
section, the envvar will be replaced correctly.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.
Good call! Done.
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.
Instead of overriding the
storage_config
, should we just replace all the env var in thefile_mounts
section? For example, in L285 we do:With that, the user can do something like the following:
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.
One worry with string replacement is that it'll make some weird things "work":
I think we should make replacement more structured.
That said, supporting env vars in src/dst seems a useful case. How about we support it when users ask?
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.
Sounds good, though I still think the replacement for
dst
andsrc
might be quite important, as that was one thing we would like to have for the Vicuna for downloading the correct llama weight from the bucket based on the envvar.For the problem you mentioned, we can do the schema validation before the replacement happens to avoid that issue.
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.
Good call! Updated as such.
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.
nice, we should have all our tests to be
--cpus 2+
in the future to save cost. ; )