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

HuggingFace improvements #649

Merged
merged 7 commits into from
Aug 16, 2023
Merged

HuggingFace improvements #649

merged 7 commits into from
Aug 16, 2023

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Aug 4, 2023

  • Log Trainer.args.

  • Add log_model argument.
    Every framework does its own thing. No strong opinions but I went with the following:

    • If None (default) will not log any artifact.
    • If all will call log_artifact with output_dir at each on_save call.
    • If True will save the model on_train_end and call log_artifact with type=model and copy=True.
      Will use best as name if args.load_best_model_at_end else last.
  • Add Notebook/Colab example

Closes #641


TODO:

  • dvc.org updates

daavoo added 3 commits August 2, 2023 12:27
- If `None` (default) will not log any artifact.
- If `all` will call log_artifact with `output_dir` at each `on_save` call.
- If `last` will save the model `on_train_end` and call `log_artifact` with type=model and copy=True.
@daavoo daavoo added feature A: frameworks Area: ML Framework integration labels Aug 4, 2023
@daavoo daavoo self-assigned this Aug 4, 2023
@daavoo daavoo requested a review from dberenbaum August 4, 2023 13:46
self.live.log_artifact(args.output_dir)
output_dir = os.path.join(args.output_dir, "last")
fake_trainer.save_model(output_dir)
self.live.log_artifact(output_dir, type="model", copy=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cross-framework consistency isn't our highest priority, but should we agree on some common principles for the final artifact, like naming and whether to copy it?

Copy link
Contributor Author

@daavoo daavoo Aug 7, 2023

Choose a reason for hiding this comment

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

I would like for all the integrations to have just 2 options:

  • all/checkpoints: resuming scenarios.
    Log the entire checkpoint folder

  • best: model registry
    Log the best checkpoint on end with copy=True, name="best", type="model"

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine with me. Do you want to update the lightning logger to use copy=True? AFAIK the rest is consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update this and lightning to use that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, are you also suggesting to change the behavior of log_model=True in lightning to track only the copied best artifact and not the whole directory? That's fine, just want to make sure I understand what you mean.

For HF, how should we handle the last/best checkpoint? If args.load_best_model_at_end, we could add name=best? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, are you also suggesting to change the behavior of log_model=True in lightning to track only the copied best artifact and not the whole directory

I think I would suggest dropping the boolean value.

For HF, how should we handle the last/best checkpoint? If args.load_best_model_at_end, we could add name=best? WDYT?

Yes, makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would suggest dropping the boolean value.

I worry doing that and/or not saving the checkpoints dir breaks consistency with mlflow/wandb/etc. in lightning for the sake of consistency across dvclive. I would probably err on the side of sticking with consistency for lightning over consistency for dvclive where they conflict, but we can always make this a follow-up PR if it is taking this off track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or HF, how should we handle the last/best checkpoint? If args.load_best_model_at_end, we could add name=best? WDYT?

Updated with this behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, dropped last option in favor of True

Copy link
Collaborator

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Have a few questions where we need to align.

As far as breaking changes, maybe we should go ahead and make the easy ones from the 3.0 checklist and do a release. I think we will need a major version where we transition to log_model and moving callbacks into frameworks and can't do it all at once anyway. The alternative is likely to branch-based development and only make the breaking changes available on the 3.0 branch. WDYT?

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.17% 🎉

Comparison is base (786c83a) 88.06% compared to head (fbf8865) 88.24%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   88.06%   88.24%   +0.17%     
==========================================
  Files          43       43              
  Lines        3042     3088      +46     
  Branches      260      270      +10     
==========================================
+ Hits         2679     2725      +46     
+ Misses        324      323       -1     
- Partials       39       40       +1     
Files Changed Coverage Δ
src/dvclive/huggingface.py 100.00% <100.00%> (+6.89%) ⬆️
tests/test_frameworks/test_huggingface.py 95.95% <100.00%> (-0.27%) ⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@SoyGema SoyGema left a comment

Choose a reason for hiding this comment

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

Thanks for making an example! 🙏Would like to propose some additions if you don´t mind. I can´t comment directly due to the notebook nature, so hopefully this can help. If you are time sensitive, and agree on the proposed additions/changes, happy to submit a contrib.🎺

  1. Add # Goal/Intro section
    Cover the full functionality the example is exploring at the beginning of the notebook

Proposal : Example of fine-tuning sentiment analysis classifier based on imbd and distilbert pretrained model , experiment tracking and results metrics analysis with dvclive and dvc api

  1. Add section # Initialize git and dvc in 2nd cell
    why? It gives context about the necessary requirements beyond the libraries install that a data scientist needs to run some other HF examples

  2. Change # Dataset for # Dataset and Tokenization in 3rd cell
    The section includes a Tokenization process

  3. Add # Evaluation metrics section
    For consistency / coherence with respect to the process

  4. Possible discussion L28
    Describe in comment what log_model does for user? which seems to be the improvement of the PR?

  5. Explain # Comparing section
    Context : unclear right now whats really going on with naming . I´ve lost a few chapters of this exploring other things . Thinking about sharing in #9709 . When exploring this, a question came into my mind, what is the difference in between dvc.api and dvclive ?

@daavoo
Copy link
Contributor Author

daavoo commented Aug 15, 2023

Thanks for making an example! 🙏Would like to propose some additions if you don´t mind. I can´t comment directly due to the notebook nature, so hopefully this can help. If you are time sensitive, and agree on the proposed additions/changes, happy to submit a contrib.🎺

Thanks @SoyGema ! All points make sense to me. I have opened a separate issue to address as a follow-up. I believe it applies to all the examples and not only to the huggingface one.

@daavoo
Copy link
Contributor Author

daavoo commented Aug 15, 2023

As far as breaking changes, maybe we should go ahead and make the easy ones from the 3.0 checklist and do a release. I think we will need a major version where we transition to log_model and moving callbacks into frameworks and can't do it all at once anyway. The alternative is likely to branch-based development and only make the breaking changes available on the 3.0 branch. WDYT?

I kept the model_file behavior with a warning about deprecation for now

@daavoo daavoo requested a review from dberenbaum August 15, 2023 09:35
@daavoo daavoo force-pushed the huggingface-log-model branch from a88e9e7 to 07dbe4e Compare August 15, 2023 10:03
@daavoo daavoo force-pushed the huggingface-log-model branch from 07dbe4e to 2c03182 Compare August 15, 2023 10:13
@daavoo daavoo merged commit f1b8e2a into main Aug 16, 2023
@daavoo daavoo deleted the huggingface-log-model branch August 16, 2023 09:31
daavoo added a commit to iterative/dvc.org that referenced this pull request Aug 18, 2023
daavoo added a commit to iterative/dvc.org that referenced this pull request Aug 18, 2023
daavoo added a commit to iterative/dvc.org that referenced this pull request Aug 18, 2023
* dvclive: Add huggingface updates

per iterative/dvclive#649

* updates from review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: frameworks Area: ML Framework integration feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HuggingFace: Add log_model
4 participants