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

[trainer] renaming cl args/ trainer attributes to be clear per-gpu vs total #9821

Closed
stas00 opened this issue Jan 26, 2021 · 3 comments
Closed

Comments

@stas00
Copy link
Contributor

stas00 commented Jan 26, 2021

As we started discussing here #9801 (comment) perhaps we could have a design session where we look at all of the trainer cl args (and their class attribute counterparts) and see which of them contain ambiguity wrt per-gpu vs total (and perhaps other important renames where we find things are confusing).

The intention is to make the API more intuitive and minimize the number of time we introduce breaking changes, but to attempt to do that in one go as much as possible.

One such item we started to discuss is --max_steps, then @sgugger mentioned --num_train_epochs and there are probably others.

I also proposed to potentially entertain creating a back-compat module to minimize the breaking changes pain where it's possible - renames fall perfectly into this category. I wrote:

In some previous projects for such things we also had a back-compat mode, which ones enabled supported a whole bunch of old ways until the user was ready to make the shift to the new code. Surely a rename of a cl arg could be easily supported by such feature. So here, instead of a deprecation cycle per item the approach is to keep anything old around but only if it's loaded from a helper module. So that the main code remains clean of deprecated things. This was in a different programming environment where it was developer, so I will have to think how to do the same here.

@LysandreJik, @patrickvonplaten, @sgugger

@github-actions
Copy link

github-actions bot commented Mar 6, 2021

This issue has been automatically marked as stale and been closed because it has not had recent activity. Thank you for your contributions.

If you think this still needs to be addressed please comment on this thread.

@stas00
Copy link
Contributor Author

stas00 commented Mar 6, 2021

I'd love to get feedback on that. Thank you!

@stas00 stas00 reopened this Mar 6, 2021
@stas00 stas00 removed the wontfix label Mar 18, 2021
@stas00
Copy link
Contributor Author

stas00 commented Mar 18, 2021

As it doesn't seem to resonate as a real need, I'm closing this one.

@stas00 stas00 closed this as completed Mar 18, 2021
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

No branches or pull requests

1 participant