-
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
Send exec status independent of report permissions #561
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
|
||
self.ui.print("> Cube execution complete") | ||
if self.allow_sending_reports: | ||
if not self.dataset.for_test: |
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 test
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.
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 ensure
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.
Good point, I'm removing this check. Also everything looks fine with the report_sender methods, so I think we're good there!
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.
Looks good to me. Let's fix typos and run tests again, if it goes smoothly, then PR is ready to be merged
Co-authored-by: Viacheslav Kukushkin <[email protected]>
Co-authored-by: Viacheslav Kukushkin <[email protected]>
Co-authored-by: Viacheslav Kukushkin <[email protected]>
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.
LGTM
status: waiting for experiment leads to decide on wether this is OK