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

moving examples out of the package #36

Closed
Borda opened this issue Aug 4, 2019 · 9 comments
Closed

moving examples out of the package #36

Borda opened this issue Aug 4, 2019 · 9 comments

Comments

@Borda
Copy link
Member

Borda commented Aug 4, 2019

Hello, nice peace of work. I was wondering if it would be easier to have examples out of the package (more intuitive to finds and keep the package simple) as well as all tests?
(e.g. pytorch-lightning/pytorch_lightning/testing_models/lm_test_module.py)

@williamFalcon
Copy link
Contributor

williamFalcon commented Aug 5, 2019

yeah, the problem was that there were issues with the test models being outside of the package.

But if you want to make the change, I do think it would be cleaner. Feel free to submit a PR! It would be super helpful!

Tests will also need to be updated.

@williamFalcon
Copy link
Contributor

williamFalcon commented Aug 5, 2019

Awesome PR, thanks for submitting! Interested in looking at any of the other issues we need help with?

@Borda

@Borda
Copy link
Member Author

Borda commented Aug 5, 2019

I was thinking about some workaround automatic testing, formation and coverage...
I will prepare another PR :)

@williamFalcon
Copy link
Contributor

amazing! That would be great. Right now a big issue is having to manually run the GPU tests for free.

I'll create an issue to track these changes there

@gregunz
Copy link

gregunz commented Oct 18, 2019

I am new to python packages, modules and stuff but I think that now that the directory examples is in the root, the setup.py detects it as a package with find_packages().

This results in anyone installing your pytorch-lightning package to also have a examples package installed into their environment.

As a matter of fact, it is preventing me from loading one of my local module named examples.

gregunz added a commit to gregunz/pytorch-lightning that referenced this issue Oct 18, 2019
@gregunz
Copy link

gregunz commented Oct 18, 2019

The above commit could solve this.
It simply excludes the directory from the package.

Note that it also excludes the tests which should be causing the same type of problems.
I will proceed with a pull request if it makes sense to you @williamFalcon @Borda.

@Borda
Copy link
Member Author

Borda commented Oct 18, 2019

I do agree with the change, or even explicitly name the 'pytorch-lightning'
Btw, why the package is imported pytorch... not just torch... such as PyTorch

@gregunz
Copy link

gregunz commented Oct 18, 2019

Whitelisting as you suggest seems a good idea BUT doing so, sub-packages will not be imported.

About the name, I also think torch-lightning makes a lot of sense.

@williamFalcon
Copy link
Contributor

let’s keep it as is for now. i don’t want to introduce breaking changes often (pytorch-lightning)

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

No branches or pull requests

3 participants