-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Update notebook.py to support multi eval datasets #25796
Conversation
fix multi eval datasets
using `black` to reformat
Note that my last commit corresponds to the workflow check. I dont think it's necessary. |
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.
Hey! Thanks for opening the fix. I think there was an issue related to this, if you could link it would be great!
To fix the CIs you can run make style
. (Will also be easier to review the required changes!)
Already looks nice, would be better if we can keep the Validation Loss
in the single dataset case!
src/transformers/utils/notebook.py
Outdated
elif ( | ||
force_update | ||
or self.first_calls > 0 | ||
or value >= min(self.last_value + self.wait_for, self.total) | ||
): |
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.
Will be fixed by make style
(same for changes that break lines
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.
Thanks for the improvement! Looks good to me, but as mentioned let's see if we can try to keep validation loss to still be a thing if we have a single dataset :)
src/transformers/utils/notebook.py
Outdated
if args.evaluation_strategy != IntervalStrategy.NO: | ||
column_names.append("Validation Loss") |
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.
If we don't have multiple datasets, let's keep this in please :)
src/transformers/utils/notebook.py
Outdated
@@ -320,7 +362,7 @@ def on_log(self, args, state, control, logs=None, **kwargs): | |||
|
|||
def on_evaluate(self, args, state, control, metrics=None, **kwargs): | |||
if self.training_tracker is not None: | |||
values = {"Training Loss": "No log", "Validation Loss": "No log"} |
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.
Same here
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
support Validation Loss
reformat
@matrix1001 do you want me to merge this or do you still have modifications to include? |
@ArthurZucker merge. |
* Update notebook.py fix multi eval datasets * Update notebook.py * Update notebook.py using `black` to reformat * Update notebook.py support Validation Loss * Update notebook.py reformat * Update notebook.py
In this code, on master branch I am still seeing:
|
@puneetdabulya This PR only fixes |
* Update notebook.py fix multi eval datasets * Update notebook.py * Update notebook.py using `black` to reformat * Update notebook.py support Validation Loss * Update notebook.py reformat * Update notebook.py
* Update notebook.py fix multi eval datasets * Update notebook.py * Update notebook.py using `black` to reformat * Update notebook.py support Validation Loss * Update notebook.py reformat * Update notebook.py
Fix key error when using multiple evaluation datasets
Code triggering the error
Any code using multiple eval_dataset will trigger the error.
Before fix
Here's detailed error msg:
After fix
Some explanation
I remove all predefined key "Validation Loss". However, this will have the following result if there is only one eval_dataset:
I think we don't have to name it as "Validation Loss"?
My modification allows dynamic columns and updates values if multiple calls for
NotebookProgressCallback.on_evaluate
correspond to the same epoch or step.