Mark trainer.data_connector as protected

[daniellepintz]

What does this PR do?

Part of #9778

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 🙃

[daniellepintz]

We are getting the Python error "Protected member accessed from outside the class", as expected. What should we do about this?

[codecov]

Codecov Report

Merging #10031 (a5d62be) into master (c3614f1) will decrease coverage by 4%.
The diff coverage is 100%.

❗ Current head a5d62be differs from pull request most recent head c3e6f15. Consider uploading reports for the commit c3e6f15 to get more accurate results

@@           Coverage Diff            @@
##           master   #10031    +/-   ##
========================================
- Coverage      92%      89%    -4%     
========================================
  Files         182      180     -2     
  Lines       16113    15870   -243     
========================================
- Hits        14896    14075   -821     
- Misses       1217     1795   +578     

[carmocca]

Where's the error?

[four4fish]

We are getting the Python error "Protected member accessed from outside the class", as expected. What should we do about this?

curious why we want to make it protected if we want to access trainer._accelerator_connector outside trainer?

[akihironitta]

Where's the error?

@carmocca DeepSource compains it. https://deepsource.io/gh/PyTorchLightning/pytorch-lightning/run/495b4588-7aa7-4aa7-b300-c1997b68c96d/python/

[daniellepintz]

@four4fish we don't want it to be part of the public API, i.e. we don't want users to use it. more details in the issue #9778.

@carmocca Regarding the DeepSource error, we can still merge the PR with this error, but it's a bit strange that we are accessing the protected member outside of the class. still I think it's best to merge this PR so that the connectors are no longer part of the public API

[tchaton]

Approving, but I am not a huge fan of this. Some users have replaced the connectors in the past to add some custom behaviours.

[awaelchli]

Same here, not a fan of this either for this reason: #9778 (comment)

Lightning is not properly respecting encapsulation in many places. Adding underscore to members does not really make them protected if we keep using them outside as if they are part of the public interface. It's an excuse to break backward compatibility (my words, my opinion, a major complaint of mine since a long time 😅 , it's really hard to get rid of though).

[daniellepintz]

@tchaton @awaelchli I hear your concerns, and just reread @awaelchli's comment on this issue. If you prefer I can mark this PR as draft until we refactor things so that we don't violate the protected rule and access it outside of trainer class.

[awaelchli]

I'm fine with moving forward if we want to change the internals of the DataConnector. But it seems we don't actually have any concrete plans (see section about Connectors in https://docs.google.com/document/d/1xHU7-iQSpp9KJTjI3As2EM0mfNHHr37WZYpDpwLkivA/edit#).

In the past we have expressed some ideas to better define and restrict the responsibilities of the connectors. I will add more ideas there in the document.

[daniellepintz]

Sounds good, I propose we move forward with this, and in parallel work on improving the connectors in the arch doc

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[tchaton]

LGTM !