-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Deprecate num_processes
,gpus
, tpu_cores
, and ipus
from the Trainer constructor
#11040
Conversation
If test is calling/using deprecated flag/methods, the test will fail. Which means, with these flags deprecate, we will have to update a lot of tests? |
@four4fish yeah, I just asked @carmocca about that in Slack but better to have the discussion here. Since we added that behavior in #9940, this PR requires updating almost every test. It is pretty overwhelming, so I am wondering if there is any way we can split it into multiple PRs/temporarily disable this functionality. |
I had the same problem when trying to land #9940. My suggestion is keep working on it here with everything together, then, split off pieces into separate PRs.
No, It was added exactly with this purpose. |
@carmocca this deprecation is very user facing. Between 1.6 and 2.0 I would expect user still actively using these flags and slowly migrate out. Do we want tests to cover the correctness if user still using these flags? Also if we rewrite accelerator_connector, how do we know the deprecated flags are work as before if there is no unit test? I think update tests should be doable, just search and replace. The regression could caused by removing these tests is what I'm concerned. |
The "deprecated" tests can stay. Failing on deprecation forces you to do one of the following:
|
Oh I see it, pytest.deprecated_call() will keep some old tests alive. Make sense! |
@daniellepintz Any updates on this PR ? Let's move the depreciation to 2.0. |
@tchaton Just updated this PR moving the deprecation to 2.0. The deprecation itself is ready to go, but I still need to fix all of the tests, which seems like it will be very time consuming, since virtually every test is affected. I will probably get to it next week. Help is welcome! |
@daniellepintz I can help! Just let me know which ones to do. |
@mathemusician thanks so much! Feel free to work on any test file, we can probably do one PR per file |
Thanks. Keep us updated on progress. |
I made a little Trello board to keep track of what's happening: https://trello.com/b/He49BfCc/pl-tests |
@carmocca @kaushikb11 we should merge this soon before it gets outdated. |
What does this PR do?
Fixes #10410
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃