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.

Sign up for GitHub

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

Jump to bottom

chore: update CLI commands to work with global APIs #10089

Merged
merged 8 commits into from
Oct 22, 2024
Merged

chore: update CLI commands to work with global APIs #10089

merged 8 commits into from
Oct 22, 2024

Conversation

amandavialva01
Copy link
Contributor

@amandavialva01 amandavialva01 commented Oct 21, 2024 •
edited by jira bot
Loading

Ticket

CM-576

Description

Update CLI commands to work with global APIs

Test Plan

CI passes (automated testing)

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@amandavialva01 amandavialva01 requested review from a team as code owners October 21, 2024 16:27
@cla-bot cla-bot bot added the cla-signed label Oct 21, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 21, 2024 •
edited
Loading

✅ Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 69fc4be
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6717c8dd60c272000754f2e6

@codecov Codecov
Copy link

codecov bot commented Oct 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 31 lines in your changes missing coverage. Please review.

Project coverage is 54.42%. Comparing base (3ca3418) to head (69fc4be).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
harness/determined/cli/config_policies.py 0.00% 31 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #10089      +/-   ##
==========================================
- Coverage   54.43%   54.42%   -0.01%     
==========================================
  Files        1262     1262              
  Lines      158901   158918      +17     
  Branches     3631     3631              
==========================================
- Hits        86494    86490       -4     
- Misses      72273    72294      +21     
  Partials      134      134
Flag Coverage Δ
backend 45.52% <ø> (-0.01%) ⬇️
harness 72.70% <0.00%> (-0.05%) ⬇️
web 53.95% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
harness/determined/cli/config_policies.py 16.36% <0.00%> (-7.33%) ⬇️

... and 5 files with indirect coverage changes

kkunapuli
kkunapuli reviewed Oct 21, 2024
View reviewed changes
e2e_tests/tests/api_utils.py Outdated Show resolved Hide resolved
harness/determined/cli/config_policies.py Outdated Show resolved Hide resolved
harness/determined/cli/config_policies.py Outdated Show resolved Hide resolved
harness/determined/cli/config_policies.py Outdated Show resolved Hide resolved
kkunapuli
kkunapuli approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

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

LGTM!

@amandavialva01 amandavialva01 enabled auto-merge (squash) October 21, 2024 20:08
jgongd
jgongd reviewed Oct 21, 2024
View reviewed changes
harness/determined/cli/config_policies.py Outdated
print(
"Successfully deleted "
+ workload_type
+ "config policies for workspace "
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency: I would be great to use f-string here too.

jgongd
jgongd approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@jgongd jgongd left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the adding tests for all the behaviors!

rb-determined-ai
rb-determined-ai reviewed Oct 21, 2024
View reviewed changes
harness/determined/cli/config_policies.py
Comment on lines +137 to +138
"-w",
"--workspace-name",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why --workspace-name and not --workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am cool with changing this back to --workspace, I guess I wanted to distinguish the param as a string rather than an int of the workspace ID, but any such misconception could easily be corrected with the det config-policies <command> help
@kkunapuli up to you! Should we go with --workspace-name or --workspace?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... well I suppose --workspace-name is not an unreasonable choice, since we take "project id" for det e create.

Ugh, but I wish we had standardized the CLI on either names or ids everywhere and this wasn't a concern. But that isn't the fault of this pr, for sure.

Copy link
Contributor Author

@amandavialva01 amandavialva01 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

but I wish we had standardized the CLI on either names or ids everywhere and this wasn't a concern.

Agreed! I think with workspaces for the most part, we take in the name in the CLI and then retrieve the ID in the command handling function before passing it to the APIs.

Hmm, looking at this comment from when the original work was merged, it seems like the suggestion from model dev was also to use --workspace-name instead of --workspace, so I changed this back!

Copy link
Contributor

@kkunapuli kkunapuli Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's stick with workspace-name. There's already precedence with notebooks

% det notebook start -h

-w WORKSPACE_NAME, --workspace-name WORKSPACE_NAME
                                          workspace name (default: None)

Copy link
Contributor Author

@amandavialva01 amandavialva01 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Gotcha!
ok I'll change this back to --workspace-name.
Users can specify --workspace as a shorthand (or any substring shorter than "workspace-name") if they want to!

rb-determined-ai
rb-determined-ai reviewed Oct 21, 2024
View reviewed changes
harness/determined/cli/config_policies.py
),
cli.Arg(
"--workspace",
"config_file",
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a breaking change?

Copy link
Contributor Author

@amandavialva01 amandavialva01 Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The det config-policies ... CLI commands have not yet been released to customers! We are aiming to release this in 0.38.0

@amandavialva01 amandavialva01 enabled auto-merge (squash) October 22, 2024 15:47
@amandavialva01 amandavialva01 merged commit 539df5e into main Oct 22, 2024
81 of 94 checks passed
@amandavialva01 amandavialva01 deleted the amanda/CM576 branch October 22, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkunapuli kkunapuli kkunapuli approved these changes

@rb-determined-ai rb-determined-ai rb-determined-ai approved these changes

@jgongd jgongd jgongd approved these changes

@maxrussell maxrussell Awaiting requested review from maxrussell maxrussell is a code owner automatically assigned from determined-ai/backend

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amandavialva01 @rb-determined-ai @kkunapuli @jgongd