Skip to content

Remove AcceleratorConnector.device_type #12077

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

Closed
DuYicong515 opened this issue Feb 24, 2022 · 7 comments · Fixed by #12081
Closed

Remove AcceleratorConnector.device_type #12077

DuYicong515 opened this issue Feb 24, 2022 · 7 comments · Fixed by #12081
Labels

Comments

@DuYicong515
Copy link
Contributor

DuYicong515 commented Feb 24, 2022

Proposed refactor

Remove the property AcceleratorConnector.device_type and migrate its usage
https://github.com/PyTorchLightning/pytorch-lightning/blob/550d3a640d1bfeb731a16b2996d3160f0f7eb071/pytorch_lightning/trainer/connectors/accelerator_connector.py#L766-L775

Alternative

Check isinstance(Trainer.accelerator, [CPU/GPU/TPU/IPU]Accelerator) to get the device type.

Motivation

Remove deprecated accelerator_connector properties and deprecate Trainer properties accordingly.

AcceleratorConnector.device_type was not used within PytorchLightning anymore and those connector properties were never meant to be public.

Remove the property AcceleratorConnector.device_type, which is not used.

Additional context

No other usage, should be a safe remove

cc @justusschock @awaelchli @rohitgr7 @four4fish

@carmocca
Copy link
Contributor

In the linked PR, I see the property was not deprecated previously but just removed.

Was this discussed somewhere? Is the assumption that we don't deprecate connector properties? Or some other reasoning?

@four4fish
Copy link
Contributor

four4fish commented Feb 25, 2022

In the linked PR, I see the property was not deprecated previously but just removed.

Was this discussed somewhere? Is the assumption that we don't deprecate connector properties? Or some other reasoning?

@carmocca this is one of the follow ups in #11449, all these properties below L768 are not used anymore and can be removed. I think you already removed the trainer.device_type?

@carmocca
Copy link
Contributor

I think you already removed the trainer.device_type?

I did, but that was prefixed by _: #11992

Personally, I'm okay with removing this. Properties and attributes inside the connectors were never meant to be public. I just wanted to raise awareness about this.

@krshrimali
Copy link
Contributor

Just did a quick google search, to see if anywhere in a blog/docs, anyone has used device_type, and found no relevant results. So looks like removal is safe, but just putting this question for someone who comes to this issue for context: What is the suggested alternate (if any)? If a user wants to know the device type of the AcceleratorConnector object, how should they do it now? (except isinstance(obj, [CPU/GPU/TPU/IPU]Accelerator))

@DuYicong515
Copy link
Contributor Author

@krshrimali Thanks for the suggestion! I feel getting the device_type string are not recommended now. (The _AcceleratorType is an internal enum instead).

The best alternative is to check isinstance(Trainer.acclerator, [CPU/GPU/TPU/IPU]Accelerator) whenever they want to know the accelerator type they are using? I've added this alternative to the issue descriptions.

Any other alternatives we should consider?

@krshrimali
Copy link
Contributor

krshrimali commented Feb 25, 2022

@krshrimali Thanks for the suggestion! I feel getting the device_type string are not recommended now. (The _AcceleratorType is an internal enum instead).

The best alternative is to check isinstance(Trainer.acclerator, [CPU/GPU/TPU/IPU]Accelerator) whenever they want to know the accelerator type they are using? I've added this alternative to the issue descriptions.

Any other alternatives we should consider?

Thanks for the response, I guess this is okay then. I didn't see any usage of device_type as such, so this is fine. Thank you!

Also, thanks for updating the description. Looks great now!

@DuYicong515 DuYicong515 changed the title Remove deprecated AcceleratorConnector.device_type Remove AcceleratorConnector.device_type Feb 25, 2022
@four4fish
Copy link
Contributor

I think you already removed the trainer.device_type?

I did, but that was prefixed by _: #11992

Personally, I'm okay with removing this. Properties and attributes inside the connectors were never meant to be public. I just wanted to raise awareness about this.

Thanks for the hands up, we have a bunch of properties in accl_conn can be removed like this. The trainer._accelerator_connector is already private, I think it's safe to remove properties in it directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants