Make tests runnable out-of-tree for help with conda-packaging #307
Conversation
|
I'm certainly okay with moving the test steps into the tango module somewhere. Maybe at |
Cool!
Then I still don't see a way how the multicore tests should be runnable out-of-tree... |
|
I guess I misunderstood how the tests need to run in this case. But that's fine, it's okay to include those files in the build if we have to. |
|
What's the problem here? It looks like the tests failed that need secrets, probably because @h-vetinari is not a member of the AI2 org. That's not a real failure though. Is there anything wrong with the multicore tests now? |
The issue is that the multicore tests cannot run out of tree due to the way they import the
|
|
Hey @h-vetinari, per the above conversation I thought we had decided to move the test steps into a proper tango submodule, like |
Yes!
Sorry, been pretty busy and only getting to this now. |
|
The 0.7.0 version of these patches now passed the test suite in conda-forge/tango-feedstock#8. :) If/once you approve this, I'll be happy to rebuild the other versions in between. |
|
PS. I needed to skip |
There was a problem hiding this comment.
Hey @h-vetinari, sorry for the slow response (Dirk is on vacation this week). I just have an organization suggestion.
tango/common/testing.py
Outdated
| @@ -145,3 +172,146 @@ def requires_gpus(test_method): | |||
| test_method | |||
| ) | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Can you move these to a separate module? I don't want these steps to necessarily be registered when someone just wants to import the TangoTestCase class.
I would suggest making tango/common/testing/ directory, and move this file to tango/common/testing/__init__.py, then have a separate file tango/common/testing/steps.py where you put these steps (don't import anything from steps.py into __init__.py).
There was a problem hiding this comment.
Thanks for the review - no worries, I was slow too!
I don't want these steps to necessarily be registered when someone just wants to import the
TangoTestCaseclass.
I don't see how that would happen except with * imports, which are discouraged anyway?
I mean I can do the change of course, I just don't really understand the gain...
There was a problem hiding this comment.
If those test steps are in the same file as TangoTestCase they will be registered when you import TangoTestCase even if you don't import those steps since every global statement in the file, including the @Step.register() statements, will be evaluated on import.
You can check this:
from tango.step import Step
assert Step.list_available() == []
from tango.common.testing import TangoTestCase
assert "float" in Step.list_available()There was a problem hiding this comment.
Actually, there is one substantial advantage of this approach - the diff gets much smaller. ;-)
There was a problem hiding this comment.
If those test steps are in the same file as
TangoTestCasethey will be registered when you importTangoTestCaseeven if you don't import those steps since every global statement in the file, including the@Step.register()statements, will be evaluated on import.
Ah, I was not aware there were some global shenanigans going on 😅
Also, it seems our responses were seconds apart. ;-)
|
Looks like there's some more imports that need to be updated in tests. |
Feel free to push into this PR. I'm not at a computer right now, and waiting for approval for CI each time drags out finding these trivial issues quite a bit 😅 |
|
Cool I'll take a look. Can you make sure you have the "Allow edits from maintainers" option checked? |
Yeah this looks good, thanks a lot for pushing through the final clean ups! |
This is the equivalent of allenai/allennlp#5560, but for tango.
I've been carrying a simpler version of this patch since the beginning of the feedstock (since conda does not run the tests in a source checkout, but for the finished package) - this version allows introduces an override also for other users along the model introduced for allennlp.
However, as of 0.7.0 this is not complete (and I've been struggling with this in conda-forge/tango-feedstock#8), because
multicore_executor_testdoes not respect this infrastructure. I'm going to need some help on the latter (unless I just unconditionally skip all tests from that module) - my first impression is thattest_fixtures.package.stepsshould be in some auxiliary module as part of tango, not in the fixtures.