Fix launch_alpamayo_model docstring: ckpt source is --config arg, not env var#98
Open
lonexreb wants to merge 1 commit intoNVlabs:mainfrom
Open
Fix launch_alpamayo_model docstring: ckpt source is --config arg, not env var#98lonexreb wants to merge 1 commit intoNVlabs:mainfrom
lonexreb wants to merge 1 commit intoNVlabs:mainfrom
Conversation
… env var
`launch_alpamayo_model` documents the fallback for `ckpt_path=None` as:
reads [policy].model_name_or_path from the TOML config
pointed to by the COSMOS_CONFIG env var.
But the actual implementation in `_read_ckpt_path_from_toml` (same file,
lines 21-42) does no such thing -- it scans `sys.argv` for the literal
`--config <path>` argument pair that the Cosmos `launch_replica.sh`
wrapper passes:
for i, arg in enumerate(sys.argv):
if arg == "--config" and i + 1 < len(sys.argv):
toml_path = sys.argv[i + 1]
break
There is no `os.environ` lookup in the file at all -- `COSMOS_CONFIG`
isn't read anywhere. A user trying to set `COSMOS_CONFIG=/path/to.toml`
and then call `launch_alpamayo_model(spec)` would hit the
`RuntimeError("No --config <path> found in sys.argv")` instead.
Fix the docstring to describe the actual source (`--config <path>` in
sys.argv, set by `launch_replica.sh`) and cross-reference the helper
that does the parsing. Also drop a stray double-space before "If".
Pure docs change. No runtime behavior change.
Signed-off-by: lonexreb <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
launch_alpamayo_modelinfinetune/rl/launcher.pydocuments the fallback forckpt_path=Noneas:But the actual implementation in
_read_ckpt_path_from_toml(same file, lines 21–42) does no such thing — it scanssys.argvfor the literal--config <path>argument pair that the Cosmoslaunch_replica.shwrapper passes:There is no
os.environlookup anywhere in the file —COSMOS_CONFIGisn't read at all. A user trying to setCOSMOS_CONFIG=/path/to.tomland calllaunch_alpamayo_model(spec)would hit theRuntimeError("No --config <path> found in sys.argv")instead.Fix
Pure docs change. No runtime behavior change.
--config <path>insys.argv, set bylaunch_replica.sh)._read_ckpt_path_from_tomlso the docstring stays anchored to the implementation.Verification
grep -n COSMOS_CONFIG finetune/rl/launcher.pyreturns nothing after this PR — confirming the env var was never read.