-
Notifications
You must be signed in to change notification settings - Fork 131
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
Logging Revamp #284
Logging Revamp #284
Conversation
@@ -48,9 +48,8 @@ def main(args): | |||
env_config = EnvConfig(token=TOKEN, cache_dir=args.cache_dir) | |||
evaluation_tracker = EvaluationTracker( | |||
output_dir=args.output_dir, | |||
hub_results_org=args.results_org, |
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.
Why did you remove this one ? There is a check at init that fails if this is not set while push_to_hub is set
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 didn't remove it, I changed the ordering for some reason (during dev). We can move it up for sure.
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.
Mhh I added it back a minute ago, I could'nt see it before
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.
Ahh, actually indeed you are right, I was looking at the most recent diff and thought I changed that.
Should have done the tests properly so that this is catched (https://github.com/huggingface/lighteval/pull/284/files#diff-97ad1010a3f9d0568387464594c6396718637ed66d355d6b3280aa1f72607ba1R121). Will change it!
What does this implement/fix? Explain your changes.
This PR revamps output saving system.
Comments
lighteval-tests
. I have created org I can give the ownership to either @clefourrier or @NathanHB.