-
Notifications
You must be signed in to change notification settings - Fork 30
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
Send exec status independent of report permissions #561
Merged
aristizabal95
merged 18 commits into
mlcommons:main
from
aristizabal95:report-independent-exec-status
Jul 10, 2024
Merged
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
8210af5
Send exec status independent of report permissions
aristizabal95 0579f39
Add note regarding exec status being sent
aristizabal95 ba93a25
Merge branch 'main' into report-independent-exec-status
aristizabal95 c93056a
Merge branch 'main' into report-independent-exec-status
aristizabal95 1ed0a9a
Remove unused variable
aristizabal95 80bbcef
Adjust tests to recent changes
aristizabal95 bbd27e3
add mockfs to test
aristizabal95 20f9f60
Mock ReportHandler
aristizabal95 3ae2c00
Mock watchdog observer
aristizabal95 067f78e
Send reports only if execution is not for testing
aristizabal95 8a9003f
Fix linter issue
aristizabal95 f92cb38
Simplify logic for not sending test reports. Add test
aristizabal95 2a09437
mock observer for new test
aristizabal95 220e59c
Merge branch 'main' into report-independent-exec-status
aristizabal95 666d84a
Update cli/medperf/commands/dataset/prepare.py
aristizabal95 5fbfe55
Update cli/medperf/commands/dataset/prepare.py
aristizabal95 73a9a10
Update cli/medperf/tests/commands/dataset/test_prepare.py
aristizabal95 c86571f
Remove unnecessary check for test dataset
aristizabal95 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
It seems you don't need this check anymore.
report_sender
expected to work well both with real data and testThere 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.
Can you check plz one more time if everything between
report_sender.start/stop/*
and_send_data()
works well for test datasets (as I believe you are more experienced in this piece of code). I believe it works well, just to explicitly ensureThere 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.
Good point, I'm removing this check. Also everything looks fine with the report_sender methods, so I think we're good there!