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

prevent empty statistics file #628

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Conversation

mhmdk0
Copy link
Contributor

@mhmdk0 mhmdk0 commented Nov 29, 2024

Fixes #623

@mhmdk0 mhmdk0 requested a review from a team as a code owner November 29, 2024 16:33
@mhmdk0 mhmdk0 had a problem deploying to testing-external-code November 29, 2024 16:33 — with GitHub Actions Failure
Copy link
Contributor

github-actions bot commented Nov 29, 2024

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

@mhmdk0 mhmdk0 had a problem deploying to testing-external-code November 29, 2024 17:11 — with GitHub Actions Failure
@mhmdk0 mhmdk0 had a problem deploying to testing-external-code December 2, 2024 21:34 — with GitHub Actions Failure
@aristizabal95
Copy link
Contributor

Looks good! Amazing work @mhmdk0! My only suggestion if possible would be to use the mocked filesystem for testing, instead of patching open and write. This is because those operations are pretty common, and scattered throughout our codebase. If future changes will call those methods in other portions of the tested function then those patches would introduce side effects that will be difficult to figure out.

You can refer to this example:

def setup_cube_fs(ents, fs):
for ent in ents:
# Assume we're passing ids, names, or dicts
if isinstance(ent, dict):
cube = TestCube(**ent)
elif isinstance(ent, int) or isinstance(ent, str) and ent.isdigit():
cube = TestCube(id=str(ent))
else:
cube = TestCube(id=None, name=ent)
meta_cube_file = os.path.join(cube.path, config.cube_metadata_filename)
meta = cube.todict()
try:
fs.create_file(meta_cube_file, contents=yaml.dump(meta))
except FileExistsError:
pass

As well as to the documentation of pyfakefs: https://pytest-pyfakefs.readthedocs.io/en/stable/usage.html#test-scenarios

If this is too cumbersome you can also leave it as is and create an issue as a reminder for later.

@mhmdk0
Copy link
Contributor Author

mhmdk0 commented Dec 4, 2024

Looks good! Amazing work @mhmdk0! My only suggestion if possible would be to use the mocked filesystem for testing, instead of patching open and write. This is because those operations are pretty common, and scattered throughout our codebase. If future changes will call those methods in other portions of the tested function then those patches would introduce side effects that will be difficult to figure out.

Much thanks @aristizabal95 ! I’ll review and finalize it soon. I really appreciate you providing both the example and the documentation.

@mhmdk0 mhmdk0 had a problem deploying to testing-external-code December 7, 2024 16:33 — with GitHub Actions Failure
@mhmdk0
Copy link
Contributor Author

mhmdk0 commented Dec 7, 2024

@aristizabal95 Just modified the tests to use fake fs instead of patching builtins.open. Thanks for informing me about this.

Copy link
Contributor

@aristizabal95 aristizabal95 left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great!

@aristizabal95 aristizabal95 merged commit 848bb72 into mlcommons:main Dec 12, 2024
6 of 7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
@mhmdk0 mhmdk0 deleted the bugFixing branch December 23, 2024 16:42
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.

[BUG] Empty statistics from a data owner causes a global error
2 participants