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

feat: Implement det cmd describe #9500

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

feat: Implement det cmd describe #9500

wants to merge 17 commits into from

Conversation

gt2345
Copy link
Contributor

@gt2345 gt2345 commented Jun 11, 2024

Ticket

MD-139

Description

Implement det cmd describe so that user can use CLI to fetch metadata for a single command.
Also fix an error related to det cmd ls --csv

Test Plan

Start multiple commands using det cmd run ...
det cmd list would return the entire list of commands
Select one of the commands and pass it's ids to det cmd describe COMMAND_ID
You should only see selected command
Passing invalid id would display Failed to display command metadata
Other arguments such as --json/csv work as expected

Checklist

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

@gt2345 gt2345 requested review from a team as code owners June 11, 2024 18:38
@cla-bot cla-bot bot added the cla-signed label Jun 11, 2024
Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 25c95cd
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/667c423f3cddad00086e8dca

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 11.11111% with 16 lines in your changes missing coverage. Please review.

Project coverage is 44.17%. Comparing base (da9025b) to head (25c95cd).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9500      +/-   ##
==========================================
- Coverage   49.79%   44.17%   -5.63%     
==========================================
  Files        1247     1205      -42     
  Lines      162235   159985    -2250     
  Branches     2888     2887       -1     
==========================================
- Hits        80793    70670   -10123     
- Misses      81270    89143    +7873     
  Partials      172      172              
Flag Coverage Δ
harness 40.85% <11.11%> (-22.90%) ⬇️
web 46.16% <ø> (ø)

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

Files Coverage Δ
harness/determined/cli/command.py 43.47% <ø> (ø)
harness/determined/cli/render.py 23.62% <50.00%> (-29.35%) ⬇️
harness/determined/cli/ntsc.py 22.96% <6.25%> (-30.92%) ⬇️

... and 200 files with indirect coverage changes

@gt2345 gt2345 requested review from ioga and NicholasBlaskey and removed request for stoksc June 13, 2024 13:42
Copy link
Contributor

@NicholasBlaskey NicholasBlaskey left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 188 to 190
if c.taskType == cmdType &&
((len(users) == 0 && len(userIds) == 0) || users[username] || userIds[userID]) &&
(len(ids) == 0 || ids[id]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is kinda hard for me to understand

maybe we could split this up into seperate if statements, like workspaceID does

@@ -158,6 +158,7 @@ func (cs *CommandService) listByType(
reqUserIDs []int32,
cmdType model.TaskType,
workspaceID int32,
reqIDs *[]string,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think I prefer reqIDs being not a pointer, to match the style of reqUsers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to be consistent, but I wanted to make it optional and use nil for non command task

@azhou-determined
Copy link
Contributor

i know the JIRA for this suggests supporting a list of IDs is a good idea, but i disagree. the use case for filtering a list of command tasks by a list of IDs seems really weak to me.

commands aren't really objects that are intended to be created and processed in large numbers or batches. you run commands usually as one-off processes for debugging or other manual tasks. i really can't imagine someone a) having enough commands that the list is large enough that filtering is necessary, and b) wanting to take the time to copy and paste a batch of UUIDs just to filter that list.

if we did want to support filtering on commands, we shouldn't do it by IDs. it'd be more sensible to accept a filter like workspace ID, or ordering parameters to sort by timestamps, etc.

filtering by IDs might make sense for a batch operation on something like checkpoints, where you often want to delete/download a known batch of IDs. but here we're just filtering them to render, and i highly doubt anyone will have more than 1 or 2 active command tasks at any point in time. seems like a silly feature to me, we don't support this for any other object, doesn't make sense to support it for commands.

my vote is that we just don't support passing multiple --ids. we can support describing a single command by ID with something like:
det cmd describe COMMAND_ID
which is in line with the rest of the CLI's conventions, and more than sufficient for this use case IMO.

@gt2345 gt2345 changed the title feat: Add --ids to det cmd list feat: Implement det cmd describe Jun 25, 2024
@gt2345
Copy link
Contributor Author

gt2345 commented Jun 25, 2024

i know the JIRA for this suggests supporting a list of IDs is a good idea, but i disagree. the use case for filtering a list of command tasks by a list of IDs seems really weak to me.

commands aren't really objects that are intended to be created and processed in large numbers or batches. you run commands usually as one-off processes for debugging or other manual tasks. i really can't imagine someone a) having enough commands that the list is large enough that filtering is necessary, and b) wanting to take the time to copy and paste a batch of UUIDs just to filter that list.

if we did want to support filtering on commands, we shouldn't do it by IDs. it'd be more sensible to accept a filter like workspace ID, or ordering parameters to sort by timestamps, etc.

filtering by IDs might make sense for a batch operation on something like checkpoints, where you often want to delete/download a known batch of IDs. but here we're just filtering them to render, and i highly doubt anyone will have more than 1 or 2 active command tasks at any point in time. seems like a silly feature to me, we don't support this for any other object, doesn't make sense to support it for commands.

my vote is that we just don't support passing multiple --ids. we can support describing a single command by ID with something like: det cmd describe COMMAND_ID which is in line with the rest of the CLI's conventions, and more than sufficient for this use case IMO.

@azhou-determined Thanks for your comments, I agree with what you said and implement det cmd describe COMMAND_ID instead

@determined-ci determined-ci requested a review from a team June 26, 2024 16:31
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants