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

Fix travis CI #218

Merged
merged 3 commits into from May 7, 2019

Conversation

Projects
None yet
3 participants
@arvoelke
Copy link
Contributor

commented Apr 26, 2019

The first commit makes our pytest.raises checks more strict. This is something I was using to debug, and isn't strictly needed by this PR, but IMO is a good thing to do.

The second commit localizes the os.chdir to be within a try-finally block. This avoids a really weird issue we were seeing as a part of #217 (e.g., see this job and look for nengo_loihi/hardware/tests/test_allocators.py::test_block_size).

Essentially... if an error happens when the model is being built, then the current working directory (cwd) is permanently changed. More specifically, an error occurs within HardwareInterface.__init__, and so __exit__ is never invoked. As a result, the directory is never changed back. This only occurs for --target loihi, and causes coverage.xml to be written to the wrong directory. Part of what made this issue weird is that coverage.xml is only written to the wrong directory if -n is omitted, but not for -n 1. This may be a subtle aspect of how pytest spins up its workers.

The second commit also adds a check to pytest's setup/teardown to make sure the directory is not changing. This test would fail on test_block_size with --target loihi if it were not for the fix.

Note this does not solve #217 (i.e., it does not make nengo-loihi process safe). The issue explained in the commit message of e9cd851 is still present. Solving this is outside the scope of PR.

Add match to all pytest.raises
This helps guard against accidentally catching the wrong
error. Now `grep "pytest.raises([a-zA-Z]*)" nengo_loihi -R`
reveals only one `AssertionError` with no corresponding message.
@hunse

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

I think we should be doing the chdir in create_io_snip, which gets called when run is called, not when the HardwareInterface is created. It has the added advantage of keeping all the snip stuff in one place. I don't think there's any need to change the directory in __init__.

@arvoelke

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Wait why do we even chdir at all? It must have been necessary at some point, but that no longer seems to be the case. Everything passes with nxsdk-0.8.0 without the chdir. I added a couple commits to demonstrate.

@hunse

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Yeah, I thought I remembered Intel trying to fix that, though I don't ever remember them conclusively saying they did. But if it works without it for 0.8, then let's just get rid of it. We can probably drop support for pre-0.8 by this point. It was definitely necessary with some earlier version.

Remove os.chdir logic
No longer necessary in more recent nxsdk versions.

@drasmuss drasmuss force-pushed the debug-travis branch from 9ca8a9d to 331c2c4 May 6, 2019

@drasmuss
Copy link
Contributor

left a comment

Added a commit to drop support for nxsdk<0.8.0, as well as updating the installation instructions and testing against 0.8.1. With that, this LGTM!

Drop support for nxsdk<0.8.0
Update installation instructions

Update hardware script to test against 0.8.1

@drasmuss drasmuss force-pushed the debug-travis branch from e42f900 to c0aba62 May 6, 2019

@drasmuss drasmuss merged commit c0aba62 into master May 7, 2019

3 checks passed

Travis CI - Branch Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 99.3% (+<.01%) compared to e9cd851
Details

@drasmuss drasmuss deleted the debug-travis branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.