Skip to content
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

Scaling config yaml #454

Merged
merged 3 commits into from
May 22, 2024
Merged

Scaling config yaml #454

merged 3 commits into from
May 22, 2024

Conversation

MPCherry
Copy link
Contributor

@MPCherry MPCherry commented May 22, 2024

Allow submitting a scaling config name alongside pipeline create.

@MPCherry MPCherry marked this pull request as ready for review May 22, 2024 12:37
Copy link
Member

@rossgray rossgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only question I have is whether scaling_config represents a scaling config ID or name? If it's just one or the other, maybe it's worth naming it scaling_config_id or scaling_config_name so that it's obvious to people adding it to their YAML? Or if we're gonna support both then should be fine to leave as is

@MPCherry
Copy link
Contributor Author

The only question I have is whether scaling_config represents a scaling config ID or name? If it's just one or the other, maybe it's worth naming it scaling_config_id or scaling_config_name so that it's obvious to people adding it to their YAML? Or if we're gonna support both then should be fine to leave as is

Its a name, I've copied the convention we used in the pipeline patch, but agree its slightly confusing.

@rossgray
Copy link
Member

The only question I have is whether scaling_config represents a scaling config ID or name? If it's just one or the other, maybe it's worth naming it scaling_config_id or scaling_config_name so that it's obvious to people adding it to their YAML? Or if we're gonna support both then should be fine to leave as is

Its a name, I've copied the convention we used in the pipeline patch, but agree its slightly confusing.

ok, I guess we'll have to document this somewhere anyway, so we can make sure we specify that it needs to be the name and not the ID

@MPCherry
Copy link
Contributor Author

The only question I have is whether scaling_config represents a scaling config ID or name? If it's just one or the other, maybe it's worth naming it scaling_config_id or scaling_config_name so that it's obvious to people adding it to their YAML? Or if we're gonna support both then should be fine to leave as is

Its a name, I've copied the convention we used in the pipeline patch, but agree its slightly confusing.

ok, I guess we'll have to document this somewhere anyway, so we can make sure we specify that it needs to be the name and not the ID

Just changed it anyway lol, for the api calls and models themselves we can stick to this convention but the config having name just makes it clearer.

Copy link
Member

@rossgray rossgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@MPCherry MPCherry merged commit 40d32ad into main May 22, 2024
5 checks passed
@MPCherry MPCherry deleted the matthew/scaling-config-yaml branch May 22, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants