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

Add HasUid and apply it to Benchmark and Hazard. #386

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Add HasUid and apply it to Benchmark and Hazard. #386

merged 2 commits into from
Jul 18, 2024

Conversation

wpietri
Copy link
Contributor

@wpietri wpietri commented Jun 27, 2024

Machinery for consistent UIDs. See the uids.py file and associated tests for documentation on how it works.

@wpietri wpietri requested a review from a team as a code owner June 27, 2024 12:29
Copy link

github-actions bot commented Jun 27, 2024

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

Copy link
Collaborator

@dhosterman dhosterman left a comment

Choose a reason for hiding this comment

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

Looks great!

@bkorycki
Copy link
Contributor

Nice! A couple of thoughts:

  • I think there needs to be some mechanism that checks that a UID is not used by any other object.
  • Also, it might be worth considering using something more structured than a dictionary to specify the UID. It feels unintuitive to provide keys that are unused for extra values. Also the ordering of keys affect the UID string.

@wpietri
Copy link
Contributor Author

wpietri commented Jul 18, 2024

Sorry, somehow forgot to merge this. To Barbara's points:

  • I think there needs to be some mechanism that checks that a UID is not used by any other object.

I think that would be cool, but for a given runtime, we can only know about instantiated objects, which is a small fraction of all the objects. Long term, we could put something in ModelLab that would guarantee uniqueness, although at first blush I'm not sure how we'd distinguish between true uses and false ones.

  • Also, it might be worth considering using something more structured than a dictionary to specify the UID. It feels unintuitive to provide keys that are unused for extra values. Also the ordering of keys affect the UID string.

I was thinking of the keys as having documentary value, and was intending that the ordering of the keys define the ordering of the value. I was also thinking that the structure might later on in building a parser. Happy to look at something besides a dict as we work on this, though.

@wpietri wpietri merged commit 8eafb12 into main Jul 18, 2024
4 checks passed
@wpietri wpietri deleted the add_uids branch July 18, 2024 21:37
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 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.

3 participants