-
Notifications
You must be signed in to change notification settings - Fork 529
[MAEB] Model Meta Revision Fix #3868
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
[MAEB] Model Meta Revision Fix #3868
Conversation
| languages=["eng-Latn"], | ||
| open_weights=True, | ||
| revision="N/A", | ||
| revision="no_revision", |
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.
I think this was added in other pr too
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.
I don't think so. I did the changes for the other models in their active PRs. These were already merged so created new one.
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.
isn't the revision: 30fcde30f19b87502b8435427b5f5068e401d5f6 and 53615c10408485422e09a12cda191a747f4bbe34
should we instead write:
30fcde30f19b87502b8435427b5f5068e401d5f6-53615c10408485422e09a12cda191a747f4bbe34 and just split the string on init?
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.
I don't think so. I did the changes for the other models in their active PRs. These were already merged so created new one.
I confused this with clap
isn't the revision:
30fcde30f19b87502b8435427b5f5068e401d5f6and53615c10408485422e09a12cda191a747f4bbe34should we instead write:
30fcde30f19b87502b8435427b5f5068e401d5f6-53615c10408485422e09a12cda191a747f4bbe34and just split the string on init?
Agree, this is good solution
KennethEnevoldsen
left a comment
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.
I don't think we want no-revision. I suggested an alternative. We might need to make an exception in tests
| languages=["eng-Latn"], | ||
| open_weights=True, | ||
| revision="N/A", | ||
| revision="no_revision", |
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.
isn't the revision: 30fcde30f19b87502b8435427b5f5068e401d5f6 and 53615c10408485422e09a12cda191a747f4bbe34
should we instead write:
30fcde30f19b87502b8435427b5f5068e401d5f6-53615c10408485422e09a12cda191a747f4bbe34 and just split the string on init?
|
Done. I've also fixed directory structure in the Results Repo. |
Merged latest changes from maeb branch and ran lint to fix formatting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The "N/A" format in revision messes up the directory structure in the Results Repo as it gets intepreted as two subdirectories.