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

Returning callback results when calling pipelines' train method #71

Conversation

VolodyaCO
Copy link
Contributor

Closes #62.

This PR is a proof of concept for returning values from callbacks which might be useful for immediate manipulation after the pipeline has been run.

@clementchadebec
Copy link
Owner

Hi @VolodyaCO, sorry to come back to you so late.
Thank you very much for this contribution.

I was wondering though if we should not save the metrics in an easily readable file such as a csv instead of storing them in a dict and returning them at the end of the training as you suggest. The other TrainingCallbacks do not indeed return anything at the on_train_end stage and thus neither does trainer.train() in most cases. Therefore, I wonder if this will create confusion for users.

Let me know what you think of this :)

@VolodyaCO
Copy link
Contributor Author

No worries @clementchadebec . I prefer not having side-effects such as saving things to a file. However, I see that to keep the changes consistent, one would want other TrainingCallbacks to return something explicitly (and at the moment they return None).

It looks as though another simple solution is not to return anything and not to save anything to a file. When using such a callback, the user would access the callback history and that's it. What do you think?

@clementchadebec
Copy link
Owner

Hi @VolodyaCO, thanks for the suggestion.
I agree with you that this seems to be the best solution to keep things consistent since the metrics would be available in the history attribute anyway.

@VolodyaCO
Copy link
Contributor Author

@clementchadebec sorry for the delay. I just removed the returns!

Copy link
Owner

@clementchadebec clementchadebec left a comment

Choose a reason for hiding this comment

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

Hi @VolodyaCO,
No worries!
Thank you so much for making these changes. Everything looks fine :)

@clementchadebec clementchadebec merged commit 38ded96 into clementchadebec:main Feb 23, 2023
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.

Loss value history
2 participants