-
Notifications
You must be signed in to change notification settings - Fork 51
make namespace configurable #23
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
Conversation
/do not merge After some testing, it looks like this will deploy the AW into the correct namespace, but attempts to deploy the ray pods into the "default" namespace. Needs more testing, before merging. |
18d125f
to
10804fc
Compare
Most Recent update should be good :) |
be48c21
to
4e30a64
Compare
OK, this has been tested and rebased on top the most recent commits and includes changes to configure the dashboard route to the correct namespace as well. |
4e30a64
to
17e36aa
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.
namespace
arg with a default of "default" also has to be added to the main() function of generate_yaml.py
, but otherwise lgtm!
17e36aa
to
927fadb
Compare
thanks @Maxusmusti, changed! |
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.
Looks good, just one more tiny addition
927fadb
to
e7a1745
Compare
env = {} | ||
|
||
outfile = generate_appwrapper(name, min_cpu, max_cpu, min_memory, max_memory, gpu, workers, template, image, instascale, instance_types, env) | ||
outfile = generate_appwrapper(name, min_cpu, max_cpu, min_memory, max_memory, gpu, workers, template, image, instascale, instance_types, namespace, env) |
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.
In the generate_appwrapper def, you have namespace as the second argument, so it probably needs to be the second argument in this call as well
e7a1745
to
88b0efd
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.
LGTM, feel free to merge!
Updated the SDK so that the namespace is more easily configurable. However, it still defaults to the "default" namespace. Also added a function
get_current_namespace
that will return the name of the the namespace you are currently working in.