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

Removed old test files, fixed tests for script features and mlperf #1291

Merged
merged 10 commits into from
Sep 22, 2024

Conversation

arjunsuresh
Copy link
Contributor

No description provided.

@arjunsuresh arjunsuresh requested a review from a team as a code owner September 17, 2024 13:09
Copy link

github-actions bot commented Sep 17, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

Copy link
Contributor

@gfursin gfursin left a comment

Choose a reason for hiding this comment

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

Interesting.

Actually, it seems that we still need all these tests here if we do changes in CM?

Is there a way to trigger cm4mlops tests if we do a change in this core CM repo?

Otherwise, if we remove all these tests, we will not be able to detect a problem with the main CM workflows when we update CM?

@arjunsuresh
Copy link
Contributor Author

That's correct @gfursin . But currently these test files are not effective as they were supposed to run only when changes are made to "cm-mlops" directory. I think we should make separate test for testing CM scripts (no need for the whole MLPerf pipeline?) when CM core changes are made.

@gfursin
Copy link
Contributor

gfursin commented Sep 17, 2024

That's correct @gfursin . But currently these test files are not effective as they were supposed to run only when changes are made to "cm-mlops" directory. I think we should make separate test for testing CM scripts (no need for the whole MLPerf pipeline?) when CM core changes are made.

I think it's still useful to test just 1 MLPerf pipeline here (no need for comprehensive MLPerf tests though) besides the code CM functionality just to avoid some obvious mistakes ;) ...

@arjunsuresh arjunsuresh changed the title Removed old test files Removed old test files, fixed tests for script features and mlperf Sep 17, 2024
@arjunsuresh
Copy link
Contributor Author

Sure @gfursin . I have added the script features test and also the R50 test. Currently some tests are failing on Windows and macOS which needs fix in the cm4mlops repository.

gfursin
gfursin previously approved these changes Sep 19, 2024
Copy link
Contributor

@gfursin gfursin left a comment

Choose a reason for hiding this comment

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

LGTM

@gfursin
Copy link
Contributor

gfursin commented Sep 19, 2024

Actually, I just saw the problem with certificates on Windows when downloading a dataset... Is it possible to fix it, please? Thanks!

@arjunsuresh
Copy link
Contributor Author

yes Grigori. That should be fixed in this PR

Copy link
Contributor

@gfursin gfursin left a comment

Choose a reason for hiding this comment

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

LGTM

@gfursin gfursin merged commit 04fc9a9 into mlcommons:master Sep 22, 2024
25 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants