fix: migrate CLI from deprecated SDK methods #8282
Conversation
✅ Deploy Preview for determined-ui canceled.
|
model: * Model.get_models -> list_models * Model.get_versions -> list_versions trial: * Trial.select_checkpoint -> list_checkpoints For Trial.select_checkpoint, argument validation that was in select_checkpoint has been copied into the function in the CLI that called it (trial::download), validating passed flags as flags rather than parameters in the parent.
wes-turner
force-pushed
the
wes/deprecated-cli-sdk
branch
from
d1e4421
to
16c3631
Compare
| smaller_is_better=args.smaller_is_better, | ||
| det = Determined(args.master, args.user) | ||
|
|
||
| if [args.latest, args.best, args.uuid].count(True) != 1: |
There was a problem hiding this comment.
compare with Trial.select_checkpoint
There was a problem hiding this comment.
too bad mypy complains about sum([args.latest, args.best, args.uuid]). But I like this .count(True) better than wrapping all the bools in int().
There was a problem hiding this comment.
also, are you basically just copy/pasting the .select_checkpoint() verification logic here?
You could move the body of .select_checkpoint() into an internal ._select_checkpoint(), where .select_checkpoint() exists only to emit the deprecation warning, and _select_checkpoint() is reusable by the CLI.
I like that strategy because it sticks to DRY, and avoids any possibility of diverging code. When select_checkpoint is removed from the SDK, you can migrate _select_checkpoint() here as you've done now, because then there's only one copy of it.
I don't feel too strongly about this, so you don't need to treat this comment as blocking.
There was a problem hiding this comment.
also, are you basically just copy/pasting the .select_checkpoint() verification logic here?
Yes.
Thanks for thinking about it. This is nice. I like this strategy, too, but minus two factors:
- I want the stack trace to come from the CLI
- I prefer the CLI errors to mention flags (
--smaller-is-better) instead of variables (smaller_is_better)
What I've chosen is worse from a tech-debt perspective, but I can't think of anything for (2) especially that localizes variable names to library-style or cli-style.
wes-turner
requested review from
MikhailKardash,
rb-determined-ai and
azhou-determined
and removed request for
MikhailKardash
harness/determined/cli/trial.py
Outdated
| from determined.common.declarative_argparse import Arg, ArgsDescription, Cmd, Group | ||
| from determined.experimental import Determined | ||
| from determined.common.declarative_argparse import Arg, ArgsDescription, Cmd, Group, string_to_bool | ||
| from determined.experimental.client import CheckpointSortBy, Determined, OrderBy |
There was a problem hiding this comment.
please stick to the google style guide when adding new imports.
There was a problem hiding this comment.
ha! I like it.
Ok, brb..
There was a problem hiding this comment.
It's more than an hour or two to migrate the whole CLI and I want to get this fix out today, so I just made the minimum change.
There was a problem hiding this comment.
sure, also arguably this fix and that migration shouldn't be in the same pr anyway
The following dependencies were migrated
model:
trial:
For Trial.select_checkpoint, argument validation that was in select_checkpoint has been copied into the function in the CLI that called it (trial::download), validating passed flags as flags rather than parameters in the parent.
Description
Test Plan
The changed CLI methods are:
Please try them each out, maybe playing with their flags. No deprecation warning should ever be printed.
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket