-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add isLiveDataset
field
#494
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Addresses #495 |
Manually correct formatting
Manually fix formatting
Manually fix file
Fix manually file
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.
Great, thanks!
ctx = self.node.ctx | ||
# For live datasets, we do not raise an error if the hashes checks fail, but | ||
# only the warning above. | ||
if ctx.is_live_dataset: |
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 add a unit test in download_test.py? Something like:
@pytest.mark.parametrize("conforms_to", CroissantVersion)
# Test the hex and base64 hash values
def test_hashes_are_not_checked_for_live_datasets(conforms_to):
with tempfile.NamedTemporaryFile(delete=False) as f:
filepath = f.name
ctx = Context(conforms_to=conforms_to, is_live_dataset=True)
metadata = Metadata(ctx=ctx, name="bar")
file_object = create_test_file_object(
name="foo",
content_url=os.fspath(filepath),
)
file_object.parents = [metadata]
download = Download(operations=operations(), node=file_object)
download()
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.
Great point, done.
# only the warning above. | ||
if ctx.is_live_dataset: | ||
return | ||
# In v0.8 only, hashes were not checked. |
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.
Instead of the comment, use not ctx.is_v0()
? (Probably this line is anterior to the creation of the function)
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.
Yes, I just moved the pre-existing line below. I think ctx.is_v0() would do the same.
No description provided.