Skip to content

scripts: clean up restart-strategy and service help output#14303

Merged
matanl-starkware merged 1 commit into
mainfrom
matanl/prod-clean-arg-help
Jun 3, 2026
Merged

scripts: clean up restart-strategy and service help output#14303
matanl-starkware merged 1 commit into
mainfrom
matanl/prod-clean-arg-help

Conversation

@matanl-starkware

Copy link
Copy Markdown
Collaborator

argparse rendered enum choices via str(), so --restart-strategy and --service
showed '{RestartStrategy.ALL_AT_ONCE,...}' / '{Service.Core,...}' rather than the
tokens users type. Add str to both enums returning the accepted token
(value / name), give the args a metavar plus a help line listing the options,
and add a concise help formatter so options render as '-j, --service SERVICE'
instead of repeating the metavar. Also fix restart_strategy_converter to catch
ValueError (Enum value lookup raises ValueError, not KeyError) so invalid input
gets the informative error instead of argparse's generic fallback.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

@cursor

cursor Bot commented Jun 3, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
CLI/help and error-message UX only in production ops scripts; parsing behavior is clarified and covered by new unit tests.

Overview
Production CLI scripts now show user-typed tokens in help and errors for --restart-strategy and --service instead of enum reprs like RestartStrategy.ALL_AT_ONCE / Service.Core. RestartStrategy and Service gain __str__ returning the accepted value/name; restart_strategy_converter catches ValueError (correct for enum-by-value lookup) so invalid strategies get a custom ArgumentTypeError listing valid options.

ApolloArgsParserBuilder drops choices= on those enums in favor of metavar plus inline “One of: …” help, and uses a new _ConciseHelpFormatter so help lines read -j, --service SERVICE rather than repeating the metavar (aligned with Python 3.13+ on older runtimes). test_enum_cli_tokens.py locks in string forms and converter error behavior.

Reviewed by Cursor Bugbot for commit c5eb3bc. Bugbot is set up for automated code reviews on this repo. Configure here.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@ron-starkware ron-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ron-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on matanl-starkware).


scripts/prod/update_config_and_restart_nodes.py line 140 at r1 (raw file):

        "--service",
        type=service_type_converter,
        choices=list(Service),

It looks like choices isn't needed, since type is run on first, (service_type_converter) and after that we have if converted_value not in choices: to decide whether to throw an error, but since we are checking the converted value, the input will always be valid (if the input was invalid we would have thrown an error in the `type' check).


scripts/prod/update_config_and_restart_nodes_lib.py line 125 at r1 (raw file):

                "--restart-strategy",
                type=restart_strategy_converter,
                choices=list(RestartStrategy),

It looks like choices isn't needed, since type is run on first, (service_type_converter) and after that we have if converted_value not in choices: to decide whether to throw an error, but since we are checking the converted value, the input will always be valid (if the input was invalid we would have thrown an error in the `type' check).

@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from 8b8ce98 to cf1868d Compare June 3, 2026 08:04
@matanl-starkware

Copy link
Copy Markdown
Collaborator Author

Addressed: removed the redundant choices=list(...) from both --restart-strategy and --service. The type converters already validate and raise informative argparse.ArgumentTypeErrors, and the metavar + "One of: ..." help still lists the valid values.

@ron-starkware ron-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ron-starkware reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on matanl-starkware).

@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from cf1868d to c0243c5 Compare June 3, 2026 08:29
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 773b823 to 85f43e8 Compare June 3, 2026 08:29
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from c0243c5 to 603bda9 Compare June 3, 2026 08:36
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 85f43e8 to 85fcb3f Compare June 3, 2026 08:36
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from 603bda9 to 3eda69b Compare June 3, 2026 11:56
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 85fcb3f to 3223758 Compare June 3, 2026 11:56
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from 3eda69b to c3e2607 Compare June 3, 2026 12:19
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 3223758 to 6ce3eed Compare June 3, 2026 12:19
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from c3e2607 to e00fb67 Compare June 3, 2026 12:22
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch 2 times, most recently from 9b72c31 to 8f348f3 Compare June 3, 2026 12:33
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch 2 times, most recently from 666fb9b to a31324f Compare June 3, 2026 13:50
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 8f348f3 to 94d2f12 Compare June 3, 2026 13:50
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 94d2f12 to a769e94 Compare June 3, 2026 14:00
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch 2 times, most recently from 438f50f to 38add1d Compare June 3, 2026 14:16
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from a769e94 to 179d2e9 Compare June 3, 2026 14:16
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from 38add1d to 5cb9f2b Compare June 3, 2026 17:46
@matanl-starkware matanl-starkware force-pushed the matanl/prod-parallel-restart branch from 179d2e9 to 852f831 Compare June 3, 2026 17:46
@matanl-starkware matanl-starkware changed the base branch from matanl/prod-parallel-restart to main June 3, 2026 18:01
argparse rendered enum choices via str(), so --restart-strategy and --service
showed '{RestartStrategy.ALL_AT_ONCE,...}' / '{Service.Core,...}' rather than the
tokens users type. Add __str__ to both enums returning the accepted token
(value / name), give the args a metavar plus a help line listing the options,
and add a concise help formatter so options render as '-j, --service SERVICE'
instead of repeating the metavar. Also fix restart_strategy_converter to catch
ValueError (Enum value lookup raises ValueError, not KeyError) so invalid input
gets the informative error instead of argparse's generic fallback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@matanl-starkware matanl-starkware force-pushed the matanl/prod-clean-arg-help branch from 5cb9f2b to c5eb3bc Compare June 3, 2026 18:01
@matanl-starkware

Copy link
Copy Markdown
Collaborator Author

@ron-starkware — next one 🙏 #14302 merged, so this rebased onto main and its Reviewable check reset. Re-approve this revision when you can? (Rebase only — no code changes since your prior approval.)

@matanl-starkware

Copy link
Copy Markdown
Collaborator Author

@ron-starkware friendly follow-up 🙏 — this one's been ready overnight (CI green, no open threads); it just needs a fresh LGTM on the rebased revision so I can merge it and continue the stack (#14304#14306 next). Thanks!

@ron-starkware ron-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ron-starkware resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on matanl-starkware).

@matanl-starkware matanl-starkware added this pull request to the merge queue Jun 3, 2026
Merged via the queue into main with commit 3107e54 Jun 3, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants