Skip to content
This repository was archived by the owner on Nov 17, 2025. It is now read-only.

Conversation

@michaelosthege
Copy link
Contributor

My understanding of best practice is that the tests should live either in their own folder outside of the actual package (that's the case now) or as a submodule that is not imported from the actual package modules.

The import tests line conflicts with both of the above.

If just removing the import doesn't work because they are too entangled, we should move the tests into a submodule?

closes #84

@twiecki
Copy link
Contributor

twiecki commented Oct 6, 2020

This makes sense to me but I'm worried we're breaking GPU and we have no tests for this. Do you know which config gets set?

@michaelosthege
Copy link
Contributor Author

This makes sense to me but I'm worried we're breaking GPU and we have no tests for this. Do you know which config gets set?

I don't know. Also there are 180 files in the tests dir so I gave up looking for it right away.
In line 179 the gpuarray submodule is imported so it might be related to that.

Do we know if GPU works?

Also, I can move the import tests into the if clause and/or raise a warning, so the next person looking into GPU support becomes aware of it?

- The 6-year old comment indicated that the import was done because of some GPU
  configuration, without providing any details
- The import actually imported the first thing called "tests" that is found in
  the PATH
@brandonwillard
Copy link
Member

brandonwillard commented Oct 7, 2020

The tests/__init__.py is empty, so it doesn't look like that import tests is auto-loading anything.

The next consideration would be for imports like from theano import tests and attribute references like theano.tests within the theano package. Regarding those, I couldn't find anything.

I did find some some broken imports from a non-existent tests.theano in the gpuarray tests, and a few direct imports from tests in theano.sparse.sandbox, but those are unrelated to #84.

brandonwillard
brandonwillard previously approved these changes Oct 7, 2020
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

After going through all the possible tests imports in the project, it looks like there was one dependency between tests and theano.gpuarray.opt. It involved the PdbBreakpoint Op (previously) defined in tests.breakpoint.

Oddly, that requirement was directly imported from tests and—as a result—didn't involve the theano.tests import in question, unless you view the theano.tests import as an initial check for the availability of the tests module. Given the old organization of the project, such a check wouldn't surprise me, so I'm assuming that was the only meaningful connection left.

@brandonwillard brandonwillard merged commit 1d3454f into master Oct 7, 2020
@brandonwillard brandonwillard deleted the fix-84 branch October 7, 2020 06:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Theano top-level tests module is not resolved when running coverage on a different project that has a "tests" module

4 participants