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

Send exec status independent of report permissions #561

Merged

Conversation

aristizabal95
Copy link
Contributor

@aristizabal95 aristizabal95 commented Mar 25, 2024

status: waiting for experiment leads to decide on wether this is OK

@aristizabal95 aristizabal95 temporarily deployed to testing-external-code March 25, 2024 19:49 — with GitHub Actions Inactive
Copy link
Contributor

github-actions bot commented Mar 25, 2024

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

@aristizabal95 aristizabal95 marked this pull request as ready for review June 13, 2024 17:26
@aristizabal95 aristizabal95 requested a review from a team as a code owner June 13, 2024 17:26

self.ui.print("> Cube execution complete")
if self.allow_sending_reports:
if not self.dataset.for_test:
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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!

VukW
VukW previously approved these changes Jun 24, 2024
Copy link
Contributor

@VukW VukW left a 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

Copy link
Contributor

@VukW VukW left a comment

Choose a reason for hiding this comment

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

LGTM

@aristizabal95 aristizabal95 merged commit 19f257d into mlcommons:main Jul 10, 2024
6 of 7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 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