Skip to content
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

Safety valve to seperate the DSE execution with benchmarking execution #334

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

srivatsankrishnan
Copy link
Contributor

@srivatsankrishnan srivatsankrishnan commented Jan 12, 2025

Summary

This PR separates the DSE execution with benchmarking execution by identifying the job type (DSE or Non-DSE (i.e., benchmarking) by dynamically parsing the TestRun object. If it's a DSE job, the agent and environment integration happens along with iterators. If it's a regular job, then it will not instantiate the agent or the environment.

Input

  • Test toml file

Output

  • if DSE ranges are provides, will insatiate agent/env <--> runner
  • if not DSE, then original execution flow

Test Plan

CI/CD

Additional Notes

def update_nested_attr(obj, attr_path, value):
"""Update a nested attribute of an object."""
attrs = attr_path.split(".")
# hot fix. Will be removed after the issue is fixed in the codebase
Copy link
Member

Choose a reason for hiding this comment

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

Which issue are you referring to in this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the discussion regarding why I wanted name field to the TestDefinition. The way we have even in several generation strategies today is hardcoded to Grok to update various flags. For now it's in handlers. But will be moved inside the environment. We can just query the name field for each test definition wherever there is nested structure.

@TaekyungHeo
Copy link
Member

@srivatsankrishnan Please fix the CI issues when you get a chance.

iteration_path = self.output_path / f"iteration_{tr.dse_iteration}"
iteration_path.mkdir(parents=True, exist_ok=True)
test_output_path = iteration_path / f"{datetime.now().strftime('%Y-%m-%d_%H-%M-%S')}" / tr.name
test_output_path.mkdir(parents=True, exist_ok=True)
Copy link
Member

Choose a reason for hiding this comment

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

I do not agree with the proposed directory structure. We need to discuss this with all core members: Srinivas, Srivatsan, and Andrei.

My proposal

  • For non-DSE jobs -> Keep the current directory structure.
results
  |--test_scenario.name
      |--Test.1
            |---0
                  |--sbatch
                  |--run.sh
  • For DSE jobs -> Follow Srivatsan's proposal, but switch the levels between iteration and test scenario name.
results
|--test_scenario.name
  |--iteration_1
      |--Test.1
            |---0
                  |--sbatch
                  |--run.sh
  |--iteration_2
      |--Test.1
            |---0
                  |--sbatch
                  |--run.sh

@srivatsankrishnan
Copy link
Contributor Author

srivatsankrishnan commented Jan 15, 2025

@srivatsankrishnan Please fix the CI issues when you get a chance.

The CI errors comes from the acceptance unit tests depends upon the final directory structure we agree on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants