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

Bug - Fix issues caused by omegaconf version update #636

Merged
merged 11 commits into from
Aug 20, 2024

Conversation

RyoYang
Copy link
Contributor

@RyoYang RyoYang commented Aug 8, 2024

Description

  • Fix executor for Benchmark Execution Without Explicit Framework Field
  • Fix mca misspell issue
  • Timeout should be None if it is not explicated

@RyoYang RyoYang requested a review from a team as a code owner August 8, 2024 06:49
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.78%. Comparing base (7af75df) to head (3039a9f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #636   +/-   ##
=======================================
  Coverage   85.78%   85.78%           
=======================================
  Files          97       97           
  Lines        6923     6923           
=======================================
  Hits         5939     5939           
  Misses        984      984           
Flag Coverage Δ
cpu-python3.6-unit-test 71.61% <100.00%> (ø)
cpu-python3.7-unit-test 71.61% <100.00%> (ø)
cpu-python3.8-unit-test 71.64% <100.00%> (ø)
cuda-unit-test 83.87% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RyoYang RyoYang enabled auto-merge (squash) August 8, 2024 15:15
@RyoYang
Copy link
Contributor Author

RyoYang commented Aug 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@cp5555 cp5555 added bug Something isn't working executor SuperBench Executor labels Aug 10, 2024
@cp5555 cp5555 requested a review from guoshzhao August 10, 2024 00:48
@RyoYang RyoYang changed the title Bug: Executor - Fix executor for Benchmark Execution Without Explicit Framework Field Bug - Fix issues caused by omegaconf version update Aug 14, 2024
@cp5555 cp5555 requested a review from abuccts August 19, 2024 08:57
@guoshzhao
Copy link
Contributor

Could you please check if:

  1. All configs used in Prod env have timeout set? https://msazure.visualstudio.com/One/_git/SuperBench?path=/config
  2. Are these two bugs related with omegaconf version update?
    If the answers are Yes, I am ok with this PR.

If the answer is No for 1, then we changed the behavior of Prod.
If the answer is No for 2, maybe we need to split to two PRs?

Thanks Yang for the efforts.

@RyoYang
Copy link
Contributor Author

RyoYang commented Aug 20, 2024

Could you please check if:

  1. All configs used in Prod env have timeout set? https://msazure.visualstudio.com/One/_git/SuperBench?path=/config
  2. Are these two bugs related with omegaconf version update?
    If the answers are Yes, I am ok with this PR.

If the answer is No for 1, then we changed the behavior of Prod. If the answer is No for 2, maybe we need to split to two PRs?

Thanks Yang for the efforts.

  1. The change addresses a bug in the previous PR where all benchmarks without a specified timeout were forcefully set to a 60-second timeout. For the production configuration, if the timeout is not set, it should default to None, meaning the benchmark should have no timeout.
  2. Yes, these changes are related to the update of the omegaconf version.
    Thanks a lot for the review.

@RyoYang RyoYang merged commit 96cc4d9 into microsoft:main Aug 20, 2024
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working executor SuperBench Executor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants