Skip to content

fix: clarify missing reme launcher env#43

Open
haosenwang1018 wants to merge 1 commit intomodelscope:mainfrom
haosenwang1018:fix/clarify-missing-reme-launch-env-v2
Open

fix: clarify missing reme launcher env#43
haosenwang1018 wants to merge 1 commit intomodelscope:mainfrom
haosenwang1018:fix/clarify-missing-reme-launch-env-v2

Conversation

@haosenwang1018
Copy link
Copy Markdown

Summary

  • fail fast with a clear error when --with-reme is used without REME_PATH / REME_SCRIPT
  • add focused launcher tests covering both all-missing and partially-missing ReMe env vars
  • clean up adjacent launcher lint issues encountered while validating the change

Testing

  • uv run python -m pytest tests/test_launcher.py -q
  • uv run ruff check launcher.py tests/test_launcher.py
  • uv run ruff format --check launcher.py tests/test_launcher.py
  • git diff --check

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request primarily focuses on code formatting improvements and the addition of environment variable validation for service launching. Key changes include standardizing string quotes, improving line-wrapping for readability, and updating the pty_launch function to raise a RuntimeError if required path or script environment variables are missing. New unit tests were added to verify this validation logic. Review feedback identifies a potential AttributeError when parsing the YAML configuration if the trainer key is missing, and points out a copy-paste error in the help text for the --db command-line argument.

Comment thread launcher.py
Comment on lines +310 to +316
exp_name = config.get("trainer").get("experiment_name")
if exp_name is None or exp_name == "read_yaml_name":
if exp_name is not None:
exp_name = exp_name.replace("|", "-")
exp_name = os.path.basename(yaml_path).replace(".yaml", "")
else:
exp_name = exp_name.replace('|', '-')
exp_name = exp_name.replace("|", "-")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

This block has a potential AttributeError on line 310. If the trainer key is missing or null in the YAML config, config.get("trainer") can be None, and None.get("experiment_name") will crash the program. The logic to determine exp_name is also unnecessarily complex and contains redundant code. The suggested change fixes the crash and simplifies the logic.

Suggested change
exp_name = config.get("trainer").get("experiment_name")
if exp_name is None or exp_name == "read_yaml_name":
if exp_name is not None:
exp_name = exp_name.replace("|", "-")
exp_name = os.path.basename(yaml_path).replace(".yaml", "")
else:
exp_name = exp_name.replace('|', '-')
exp_name = exp_name.replace("|", "-")
trainer_config = config.get("trainer") or {}
exp_name = trainer_config.get("experiment_name")
if exp_name is None or exp_name == "read_yaml_name":
exp_name = os.path.basename(yaml_path).replace(".yaml", "")
else:
exp_name = exp_name.replace("|", "-")

Comment thread launcher.py
required=False,
help='Path to configuration file'
parser.add_argument(
"--db", type=str, default="", required=False, help="Path to configuration file"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The help text for the --db argument is "Path to configuration file", which appears to be a copy-paste error from the --conf argument. Based on its usage, this argument is for setting debug tags. Please update the help text to reflect its actual purpose.

Suggested change
"--db", type=str, default="", required=False, help="Path to configuration file"
"--db", type=str, default="", required=False, help="Enable debug mode and set debug tags",

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.

1 participant