hf: warn of deprecating internal callback #740
Conversation
dberenbaum
added
⌛ status: wait-core-merge
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #740 +/- ##
==========================================
+ Coverage 88.79% 95.53% +6.73%
==========================================
Files 53 55 +2
Lines 3357 3513 +156
Branches 294 312 +18
==========================================
+ Hits 2981 3356 +375
+ Misses 337 110 -227
- Partials 39 47 +8 ☔ View full report in Codecov by Sentry. |
dberenbaum
removed
⌛ status: wait-core-merge
|
Docs PR: iterative/dvc.org#4994 The dvclive callback has now been released in both upstream packages:
This one is ready to merge |
… into hf-deprecation-warning
|
Tests need an updated version of transformers from huggingface/transformers#27983 to pass |
|
@shcheklein A new hf transformers release was finally published and all tests are now passing. Could you take a look so we can get this merged please? |
examples/DVCLive_HuggingFace.ipynb
Outdated
| }, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from dvclive.huggingface import DVCLiveCallback\n", |
There was a problem hiding this comment.
q: is it expected that we use our own callback in this example?
There was a problem hiding this comment.
thanks @dberenbaum ! One item caught my attention - using an internal callback in the notebook. I didn't check also the details of the code.
Fixed that but also found a new issue and will now need to wait for huggingface/transformers#28653 and one more release for the added test to pass |
| from transformers.integrations import DVCLiveCallback as ExternalCallback | ||
|
|
||
| from dvclive.huggingface import DVCLiveCallback | ||
| from dvclive.huggingface import DVCLiveCallback as InternalCallback |
There was a problem hiding this comment.
Test old callback (inside dvclive) and new callback (inside transformers)
dberenbaum
force-pushed
the
hf-deprecation-warning
branch
from
48a1f7c to
8f2f91b
Compare
| trainer = Trainer( | ||
| model, | ||
| args, | ||
| train_dataset=data[0], | ||
| eval_dataset=data[1], | ||
| compute_metrics=compute_metrics, | ||
| ) | ||
| callback = DVCLiveCallback() | ||
| live = callback.live |
There was a problem hiding this comment.
callback.live is not initialized yet in the external callback
| @@ -125,8 +126,6 @@ def test_huggingface_integration(tmp_dir, model, args, data, mocker): | |||
|
|
|||
| logs, _ = parse_metrics(live) | |||
|
|
|||
| assert len(logs) == 10 | |||
There was a problem hiding this comment.
This does not seem like a good test to have in dvclive since it relies on how many metrics hf automatically logs, including system metrics over which dvclive has no control.
| if log_model == "last": | ||
| if log_model is True: |
There was a problem hiding this comment.
This condition was never met since the expected value is True, not "last"
| @@ -164,11 +175,12 @@ def test_huggingface_log_model(tmp_dir, model, data, mocker, log_model, best): | |||
| expected_call_count = { | |||
| "all": 2, | |||
| True: 1, | |||
| False: 0, | |||
There was a problem hiding this comment.
False is also a valid value for log_model
|
|
||
|
|
||
| @pytest.mark.parametrize("report_to", ["all", "dvclive", "none"]) | ||
| def test_huggingface_report_to(model, report_to): |
There was a problem hiding this comment.
Since there are no real tests for the callbacks in transformers, added a test here to ensure report_to adds the dvclive callback when expected
|
Will take a look after the tests are passing 🙏🏻. |
👍 Tests pass on the upstream version of |
|
@mattseddon The next release of |
src/dvclive/huggingface.py
Outdated
| @@ -24,6 +24,11 @@ def __init__( | |||
| log_model: Optional[Union[Literal["all"], bool]] = None, | |||
| **kwargs, | |||
| ): | |||
| logger.warning( | |||
| "This callback will be deprecated in DVCLive 4.0 in favor of" | |||
There was a problem hiding this comment.
[Q] Do you want to deprecate it now and remove it in 4.0?
Adds a deprecation notice about the internal HF callback and suggests to use the new external callback, which will be available in the next release of https://github.com/huggingface/transformers.
Also adds the external callback to tests since there weren't similar tests in the transformers library, and updates the example notebook.