-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parser for DSE #321
Merged
Merged
Parser for DSE #321
Conversation
This file contains 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
TaekyungHeo
requested changes
Jan 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review whether all comments are necessary. If not, remove them, leaving only the essential ones.
|
amaslenn
reviewed
Jan 6, 2025
amaslenn
reviewed
Jan 6, 2025
srivatsankrishnan
force-pushed
the
dse_parser
branch
from
January 6, 2025 17:30
041f4c7
to
74079b2
Compare
Added more unit tests for 1st point. Regarding the USER_GUIDE, yes, will be added after the End-to-End integration. Before that and doing it in this PR is premature. |
amaslenn
reviewed
Jan 7, 2025
…e of the variables that is list.
…axtookbox definitions
amaslenn
reviewed
Jan 8, 2025
src/cloudai/systems/slurm/strategy/slurm_command_gen_strategy.py
Outdated
Show resolved
Hide resolved
TaekyungHeo
previously approved these changes
Jan 8, 2025
amaslenn
reviewed
Jan 9, 2025
amaslenn
approved these changes
Jan 9, 2025
TaekyungHeo
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
As discussed and converged before the break, here is the first PR that implements the parser for DSE. The ranges will be in the Test TOML instead of test scenario as discussed. The definitions of all the variables will be under the cmd_args.
[Update]: Based on the discussions with @amaslenn, The CmdArgs type is used throughput instead of having a seperate dict of str for DSE. The downside is since
cmd_args
is used pretty much in all the workloads, the type definitions/function headers/signatures has to be defined correctly. Bulk of the changes you see in the files are fixing the types forcmd_args
to make pyright/pytest happy.The definition of the test toml when we need to specify the ranges.
The definition of test TOML when we need to specify the static ranges (as how it is defined currently).
The parser will now support both specification of static values as well as list. The downstream logic will determine how it wants to use the cmd_args. For instance, if we need to enumerate all the possible values in a list (if specified), the downstream logic will implement an iterator to manipulate the cmd_args accordingly. This specification is typically handled by the agent or the environment itself. But the parser responsibility is to ensure that it parses and retains the value ranges.
Test Plan
CI/CD.
[Update]: Removing these tests based on discussion today in design meeting. For history with previous commit hash, retaining these info. The rationale was unit test that doesn't improve coverage is not useful. I would expect the code coverage for the unit test for future PRs as a requirement to determine usefulness of any unit tests.
Specifically we add the following tests for the parser logic modifications.
cmd_args
with ranges are correctly parsed and retain lists. Asserts that thecmd_args
in theConcreteTestDefinition
instance match the expected values. Creates aTest
instance and asserts thatcmd_args
are the same asraw_cmd_args
.cmd_args
with static values are correctly parsed. Asserts that thecmd_args
in theConcreteTestDefinition
instance match the expected values. Creates aTest
instance and asserts thatcmd_args
are the same asraw_cmd_args
.cmd_args
contain ranges. Asserts that the generated commands match the expected commands for all combinations.cmd_args
contain only static values. Asserts that the generated commands match the expected single command.cmd_args
. Asserts that no commands are generated.TestParser
raises an error for TOML data missing required fields. Asserts that aValidationError
is raised when required fields are missing.TestDefinition
raises an error for unexpected fields in TOML. Asserts that aValidationError
is raised for extra fields.Using Existing Grok TestDefinition in CloudAI
GrokTestDefinitionWrapper
with all static values forcmd_args
.GrokTestDefinitionWrapper
with a mix of static and list values forcmd_args
.GrokTestDefinitionWrapper
with all list values forcmd_args
.GrokTestDefinitionWrapper
with static FDL values and list XLA flags.GrokTestDefinitionWrapper
with various types (single values and lists).GrokTestDefinitionWrapper
with incorrect types to ensure validation errors.Real System Testing
Tested on
sanity-grok-proxy-1
model on IL1.Additional Notes
Local linting is passing but failing with upstream CI/CD. The pytest failing for the golden checks for acceptance test in sbatch generation. [Update]: This is fixed.