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

package "tests" in Python path? #10335

Closed
t-vi opened this issue Nov 3, 2021 · 6 comments · Fixed by #7614
Closed

package "tests" in Python path? #10335

t-vi opened this issue Nov 3, 2021 · 6 comments · Fixed by #7614
Assignees
Labels
bug Something isn't working help wanted Open to be worked on priority: 2 Low priority task

Comments

@t-vi
Copy link
Contributor

t-vi commented Nov 3, 2021

🐛 Bug

PyTorch Lightning install its tests into "test" in the Python path. I would venture that this is not ideal.

To Reproduce

Install PyTorch Lightning

import pathlib
import pytorch_lightning
print(list((pathlib.Path(pytorch_lightning.__file__).parent.parent / 'tests').glob('*')))

This is with git a checkout from today.

@t-vi t-vi added bug Something isn't working help wanted Open to be worked on labels Nov 3, 2021
@t-vi t-vi changed the title package "test" in Python path? package "tests" in Python path? Nov 3, 2021
@Borda
Copy link
Member

Borda commented Nov 3, 2021

I thought we fixed it, but probably we need to revisit it :]

@Borda Borda added the priority: 2 Low priority task label Nov 3, 2021
@carmocca
Copy link
Contributor

carmocca commented Nov 4, 2021

I tried to fix this in #7614 but got blocked by our examples having a dependency on the tests. This comment summarizes the problem #7614 (comment).

I initially pursued the option of moving these components to the source directory so everything can access it: #8776
but dropped it after thinking that a better approach was to get rid of our pl_examples directory: #8776 (comment), Lightning-AI/tutorials#71

@Borda still waiting on your thoughts :P

@t-vi
Copy link
Contributor Author

t-vi commented Nov 4, 2021

To my mind, moving them to pytorch_lightning.testing or, if you want to emphasize they are not for general consumption, pytorch_lightning._testing (with an underscore) would be relatively standard. Doing the latter would seem to leave room for further change if any more enlightened plan comes up...
Of course, pl_examples might be good to move out of the top level, too, but the tests directory seems like a very generic thing to install in the general namespace.

@t-vi
Copy link
Contributor Author

t-vi commented Nov 4, 2021

The other (not terribly related) thing is the default data dir for pl_examples, but I guess it's better in a separate issue, as it is not obvious where to put it (personally, I'd look at the hub dir - os.path.join(torch.hub.get_dir(), 'pl_examples')).

@Borda
Copy link
Member

Borda commented Nov 4, 2021

I think that the pl_examples shall be completely independent on tests and we shall drop tests from the package as it can easily happen that other package accidentally writes their own tests in the site-packages and so make our examples unexecutable... there is no reason to distribute PL tests within the package

@Borda Borda self-assigned this Nov 4, 2021
@flying-sheep
Copy link

flying-sheep commented Nov 8, 2021

as it can easily happen that other package accidentally writes their own tests in the site-packages

Soo many packages using setuptools do that accidentally. Switching to less legacy build systems helps …

I always notice it when running my own packages’ tests and PyCharm jumps into site-packages/tests. Then I have to dig through the test code to figure up which package fucked up this time and write them an issue …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Open to be worked on priority: 2 Low priority task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants