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

Deprecate num_processes,gpus, tpu_cores, and ipus from the Trainer constructor #11040

Merged
merged 37 commits into from Apr 10, 2022

Conversation

daniellepintz
Copy link
Contributor

@daniellepintz daniellepintz commented Dec 11, 2021

What does this PR do?

Fixes #10410

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

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:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@four4fish
Copy link
Contributor

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?

@daniellepintz
Copy link
Contributor Author

@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.

@carmocca
Copy link
Member

carmocca commented Dec 15, 2021

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.
These pieces can be chosen logically per file/directory, or just by size (no more than 200 lines for example)
Then once the main PR is small enough, share it for reviews.

temporarily disable this functionality.

No, It was added exactly with this purpose.

@four4fish
Copy link
Contributor

four4fish commented Dec 15, 2021

@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.

@carmocca
Copy link
Member

The "deprecated" tests can stay. Failing on deprecation forces you to do one of the following:

  • Update the test to the new alternative
  • Add a pytest.deprecated_call() wrapping the deprecated usage. This allows you to still test the deprecated functionality. The benefit is that it is now tested explicitly and we can easily find where it's still tested once removal.

@four4fish
Copy link
Contributor

four4fish commented Dec 15, 2021

Oh I see it, pytest.deprecated_call() will keep some old tests alive. Make sense!

@tchaton
Copy link
Contributor

tchaton commented Dec 21, 2021

@daniellepintz Any updates on this PR ? Let's move the depreciation to 2.0.

@daniellepintz
Copy link
Contributor Author

@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!

@mathemusician
Copy link
Contributor

@daniellepintz I can help! Just let me know which ones to do.

@daniellepintz
Copy link
Contributor Author

@mathemusician thanks so much! Feel free to work on any test file, we can probably do one PR per file

@tchaton
Copy link
Contributor

tchaton commented Jan 4, 2022

@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!

Thanks. Keep us updated on progress.

@mathemusician
Copy link
Contributor

I made a little Trello board to keep track of what's happening: https://trello.com/b/He49BfCc/pl-tests

@mergify mergify bot removed the has conflicts label Mar 29, 2022
tests/accelerators/test_tpu.py Outdated Show resolved Hide resolved
tests/trainer/properties/test_auto_gpu_select.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Mar 29, 2022
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Apr 7, 2022
@awaelchli
Copy link
Member

@carmocca @kaushikb11 we should merge this soon before it gets outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation ready PRs ready to be merged trainer: argument
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Future of gpus/ipus/tpu_cores with respect to devices