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

Logging Revamp #284

Merged
merged 20 commits into from
Sep 4, 2024
Merged

Logging Revamp #284

merged 20 commits into from
Sep 4, 2024

Conversation

hynky1999
Copy link
Collaborator

@hynky1999 hynky1999 commented Aug 29, 2024

What does this implement/fix? Explain your changes.

This PR revamps output saving system.

  • Adds fsspec support for output directory
  • Fixes bug as the save_details parameter was never used or passed (now it's used)
  • Since one can now directly push results/details to hub, I reduce the push_x_to_hub to just push_hub, which behaves just like the push_details_to_hub was working before
  • Fixes the task_name extraction so that it doesn't explode once we move to year 2025 🫠
  • Adds some tests for checking that the evaluation results are saved correctly, I ommited tests for tensorbard logging as I haven't changed anything there
    Comments

  • The tests now require HF_TOKEN, which can write/read in lighteval-tests. I have created org I can give the ownership to either @clefourrier or @NathanHB.
  • Having secrets accessible during tests is big security risk especially when the tests can run without any interaction on PRs, but if the token has only permission to the lighteval-tests org I think it's fine
  • We should probably first merge a PR which gives ownership to lighteval over the lighteval config. Right now I can adjust the config for lighteval for nanotron path to reflect the new api. You can review the PR in the meantime but I added todo, so that we don't forget that. PS: That PR doesn't exist yet.

@hynky1999 hynky1999 requested a review from NathanHB August 29, 2024 14:35
@@ -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,
Copy link
Member

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

Copy link
Collaborator Author

@hynky1999 hynky1999 Sep 2, 2024

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.

Copy link
Member

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

Copy link
Collaborator Author

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!

@hynky1999 hynky1999 requested a review from NathanHB September 3, 2024 15:57
@hynky1999
Copy link
Collaborator Author

@NathanHB I will update this once #285 is merged to have use the new logging api

tests/fixtures.py Outdated Show resolved Hide resolved
@NathanHB NathanHB merged commit e0af821 into main Sep 4, 2024
2 checks passed
@guipenedo guipenedo mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants