Skip to content

Add override flag#50

Merged
dmjoy merged 4 commits into
mainfrom
override-flag-cli
Apr 15, 2026
Merged

Add override flag#50
dmjoy merged 4 commits into
mainfrom
override-flag-cli

Conversation

@b-fenelon

@b-fenelon b-fenelon commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Not an urgent feature

Following Jon's recommendation to #45, created an '--override' flag that accepts a YAML-like input. This adds a replace method to EvaluationCard that enables programmatic changes to multiple symbol values, which is useful for testing. This only allows value (must preserve type) and sweep replacements, limiting the changes possible without updating the card itself.

I am currently omitting a README section because I don't think we should recommended this approach over explicitly defining a new card. Once we have a standard output format, the results should capture the necessary details anyways to recreate the card with overrides.

Example usage:
magnet evaluate magnet/cards/llama.yaml --override "{helm_runs_path: ./other-data-path/benchmark_output, threshold: 0.01}"

@b-fenelon b-fenelon changed the title Add replace for symbol values Add override flag Apr 1, 2026
@dmjoy

dmjoy commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

LGTM

@b-fenelon

b-fenelon commented Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

@Erotemic Not a critical feature. Let me know if you have any thoughts (mostly follows your example from #45)

Comment thread magnet/evaluation.py
self.symbols[key]['value'] = value
elif 'sweep' in self.symbols[key]:
# accept n entries
if isinstance(value, list):

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.

This is the only thing that I see that raises a red flag. The flag is doing an "override", but this is extending an existing list and there is no way to tell it to use a new list instead. If this is needed there is some trickery that can be done. You could set the value to be a dictionary with one key like __extend__: [1, 2, 3] and that very particularly special case could be interpreted as wanting to extend the sweep list, and it would be explicit in the CLI (at least if you had knowledge of the behavior, but otherwise it looks weird enough to at least cue someone into some sort of special behavior happening)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm in favor of dropping the append. This flag is mostly for debugging and only works for python symbols (i.e. not kwdagger components). I originally didn't want to override lists to dissuade users from using '--override' in place of creating a new card. But, I agree that it's a weird behavior. If users want more flexible sweep handling, we can revisit the solution you give here

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.

@Erotemic it looks like Ben addressed the issue here, are we good to merge?

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.

Yeah, go for it.

@dmjoy
dmjoy merged commit 4be89b8 into main Apr 15, 2026
18 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