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

Remove tests dir from distribution #7596

Closed
wants to merge 1 commit into from

Conversation

flying-sheep
Copy link

You’re using find_packages wrong. It expects package paths (with .) not file paths.

Docs: https://setuptools.readthedocs.io/en/latest/userguide/package_discovery.html#using-find-or-find-packages

@Borda Borda enabled auto-merge (squash) May 18, 2021 18:36
@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #7596 (3e73d9d) into master (b214442) will decrease coverage by 3%.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##           master   #7596    +/-   ##
=======================================
- Coverage      91%     88%    -3%     
=======================================
  Files         204     199     -5     
  Lines       13630   13017   -613     
=======================================
- Hits        12414   11432   -982     
- Misses       1216    1585   +369     

@Borda Borda added ready PRs ready to be merged bug Something isn't working labels May 18, 2021
Copy link
Member

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😃

@carmocca
Copy link
Contributor

noob question: what's the relation to MANIFEST.in? The tests should be excluded:

https://github.com/PyTorchLightning/pytorch-lightning/blob/20f63377f81f4771d3f128f979b3a0f9b8d219a7/MANIFEST.in#L70

Does packages take precedence?

@Borda This might be the fix for https://github.com/PyTorchLightning/internal-dev/issues/132

@Borda
Copy link
Member

Borda commented May 19, 2021

hmm it is strange for me too, I thought the Manifest is sufficient, but maybe the combination with find_package?

@carmocca carmocca mentioned this pull request May 19, 2021
9 tasks
@flying-sheep
Copy link
Author

IDK, I no longer try to understand setuptools, and instead use flit or poetry for all my packages. It doesn’t look like you have a build step or some special behavior in setup.py, so that might be a possible choice for you too.

@stale
Copy link

stale bot commented Jun 3, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Jun 3, 2021
@stale stale bot removed the won't fix This will not be worked on label Jun 4, 2021
@awaelchli awaelchli modified the milestones: v1.4, v1.3.x Jun 4, 2021
@carmocca
Copy link
Contributor

carmocca commented Jun 4, 2021

GPU tests fail because now that we don't distribute the tests, our examples fail since they rely on our MNIST implementation.

ModuleNotFoundError: No module named 'tests.helpers.datasets'

If possible, I don't think the examples should do that at all as then it's not as easy for users to copy-paste the code and get it running.

But on the other hand, then CI can fail if downloading from torchvision fails.

@Borda save us pls

@awaelchli awaelchli removed the ready PRs ready to be merged label Jun 8, 2021
@awaelchli awaelchli added the help wanted Open to be worked on label Jun 8, 2021
@Borda
Copy link
Member

Borda commented Jun 9, 2021

@flying-sheep mind rebase on master?

auto-merge was automatically disabled June 9, 2021 08:02

Head branch was pushed to by a user without write access

@flying-sheep
Copy link
Author

Here you go

@flying-sheep
Copy link
Author

So what’s up here? Doesn’t seem like your CI feels like playing along?

@Borda Borda modified the milestones: v1.3.x, v1.4 Jul 6, 2021
@edenlightning edenlightning modified the milestones: v1.4, v1.3.x Jul 6, 2021
@mergify mergify bot added the ready PRs ready to be merged label Jul 19, 2021
@carmocca
Copy link
Contributor

Closing as this is blocked by CI limitations. Will be continued in #7614

@carmocca carmocca closed this Jul 19, 2021
@flying-sheep flying-sheep deleted the patch-2 branch October 18, 2021 13:34
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 ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants