Skip to content

Conversation

mohamedzeidan2021
Copy link
Collaborator

Previously, there were 16 different parameter inout formats across commands, creating poor user experience where users had to remember different syntaxes for different parameter types.

  • Lists: '[python, train.py]' vs "cluster1,cluster2" vs "['STATUS1', 'STATUS2']"
  • Dictionaries: Various JSON-only requirements with different validation rules
  • Objects: Only key=value,key2=value2 format supported
  • Inconsistent error messages and help text across commands

Solution

Unified Parsing System

Created a centralized parameter parsing module that standardizes JSON as base format while providing simpler syntax alternatives for common use cases

Simple Parameter Type Syntaxes

  • Lists: '["python", "train.py"]' | '[python, train.py]'
  • Dicts: '{"VAR": "value"}' | '{VAR: value}'
  • '{"name": "vol1"}' + Multiple flags | 'name=vol1,type=hostPath' (unchanged)

Implementation

  • Try JSON firstFall back to simple syntaxProvide helpful error guidance
  • Move all existing parsers__ across 6 CLI modules to use unified system
  • Update help text to show both supported formats
  • Maintain 100% backward compatibility

Tests

TestListParameters (13 tests)

Tests the parse_list_parameter() function to ensure it correctly handles both JSON array format (["item1", "item2"]) and simple list format ([item1, item2]).

TestDictParameters (13 tests)

Tests the parse_dict_parameter() function to validate parsing of both JSON object format ({"key": "value"}) and simple dictionary format ({key: value}).

TestComplexObjectParameters (12 tests)

Tests the parse_complex_object_parameter() function which supports JSON objects, JSON arrays, key-value strings (key=value,key2=value2), and multiple flag usage.

TestCommaSeparatedList (5 tests)

Tests the legacy parse_comma_separated_list() function to ensure backward compatibility with the old comma-separated format (item1,item2,item3).

TestEdgeCases (7 tests)

Tests challenging scenarios including Unicode characters, special symbols, nested quotes, file paths, and whitespace handling across all parameter types.

TestErrorMessageQuality (5 tests)

Tests that error messages are user-friendly and informative, containing parameter names, format examples, and clear guidance for invalid input.

TestBackwardCompatibility (3 tests)

Tests that all existing parameter formats from the old system continue to work unchanged, ensuring zero breaking changes.

Total: 61 tests providing comprehensive validation of the unified parameter parsing system.

PR Approval Steps

For Requester

  1. Description
    • Check the PR title and description for clarity. It should describe the changes made and the reason behind them.
    • Ensure that the PR follows the contribution guidelines, if applicable.
  2. Security requirements
  3. Manual review
    1. Click on the Files changed tab to see the code changes. Review the changes thoroughly:
      • Code Quality: Check for coding standards, naming conventions, and readability.
      • Functionality: Ensure that the changes meet the requirements and that all necessary code paths are tested.
      • Security: Check for any security issues or vulnerabilities.
      • Documentation: Confirm that any necessary documentation (code comments, README updates, etc.) has been updated.
  4. Check for Merge Conflicts:
    • Verify if there are any merge conflicts with the base branch. GitHub will usually highlight this. If there are conflicts, you should resolve them.

For Reviewer

  1. Go through For Requester section to double check each item.
  2. Request Changes or Approve the PR:
    1. If the PR is ready to be merged, click Review changes and select Approve.
    2. If changes are required, select Request changes and provide feedback. Be constructive and clear in your feedback.
  3. Merging the PR
    1. Check the Merge Method:
      1. Decide on the appropriate merge method based on your repository's guidelines (e.g., Squash and merge, Rebase and merge, or Merge).
    2. Merge the PR:
      1. Click the Merge pull request button.
      2. Confirm the merge by clicking Confirm merge.

@mollyheamazon
Copy link
Collaborator

mollyheamazon commented Sep 5, 2025

I also have changes in the init_utils with regard to parsing logic in this PR: https://github.com/aws/private-sagemaker-hyperpod-cli-staging/pull/241.
Check out my type_handler in this doc: https://quip-amazon.com/mO0UAGKOoClK/HyperPod-Template-Versioning#temp:C:ZFC34fb9209e27e449b8802722ff. It resolves parsing logic across CLI command, config.yaml and pydantic model field.
Lists: '["python", "train.py"]' | '[python, train.py]' is included.

I think your change can be made following my change to expand the support for

  • Dicts: '{"VAR": "value"}' | '{VAR: value}'
  • '{"name": "vol1"}' + Multiple flags | 'name=vol1,type=hostPath' (unchanged)

Also the generate_click_command function could be shared across inference_utils and training_utils (and maybe init_utils, but may be a stretch)

Copy link
Collaborator

@mollyheamazon mollyheamazon left a comment

Choose a reason for hiding this comment

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

Needs further examination after this PR is merged: https://github.com/aws/private-sagemaker-hyperpod-cli-staging/pull/241

Comment on lines +128 to +131
- JSON single object: '{"key": "value", "key2": "value2"}'
- JSON array (if allow_multiple=True): '[{"key": "value"}, {"key2": "value2"}]'
- Key-value single: 'key=value,key2=value2'
- Multiple flags (if allow_multiple=True): --param obj1 --param obj2
Copy link
Member

Choose a reason for hiding this comment

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

if we are supporting muliple formats, whats the recommendation via help? Is there room for help choosing a certain pattern for recommendation. Say volume example shows: key=value,key2=value2

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