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

Support NxSDK 0.9.1 #272

Merged
merged 3 commits into from
Apr 9, 2020
Merged

Support NxSDK 0.9.1 #272

merged 3 commits into from
Apr 9, 2020

Conversation

tbekolay
Copy link
Member

This is the commit from #266 supporting the next version of NxSDK. Since we're not sure when that next version will be released, I separated it out into a separate PR to deal with later.

@hunse
Copy link
Collaborator

hunse commented Apr 7, 2020

All tests pass on NxSDK 0.9.5.rc1.

@hunse hunse force-pushed the nxsdk-091 branch 2 times, most recently from 6f82ab8 to 8f51384 Compare April 7, 2020 19:24
Copy link
Member

@drasmuss drasmuss left a comment

Choose a reason for hiding this comment

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

Two minor documentation fixups and one question

and nxsdk_version is not None
and nxsdk_version < parse_version("0.9.1")
):
pytest.skip("Pop32 multichip test requires NxSDK >= 0.9.5")
Copy link
Member

Choose a reason for hiding this comment

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

The string says 0.9.5 but the check above is for 0.9.1, which one is correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm being a little tricky. First off, there hasn't been a full 0.9.5 release, so the current release is 0.9.5-daily-something, which is less than 0.9.5. So I couldn't use that. Also, there was a 0.9.1 release planned, and I think the fix for this was already in that, so I also wanted to allow that. But I'd also be fine with anything else that lets the current daily build work. We could even put it in exactly if we want; it's "0.9.5-daily-20191223" (just make sure to use parse_nxsdk_version).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we could just change the skip message to say 0.9.1. I just didn't want to confuse anyone, since it was never really released.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect there's never going to be a 0.9.1 release at this point. But I'm also kind of reluctant to specify daily build numbers in our tests. What about changing the condition to nxsdk_version < parse_version("0.9.5.dev0")? That will let the daily builds work, and any future releases >=0.9.5. Technically someone could try to run the tests with a daily build < 20191223, but I doubt anyone is running these tests other than us, and I don't know why we would ever install an old daily build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

@hunse
Copy link
Collaborator

hunse commented Apr 8, 2020

Fixups look good!

hunse and others added 3 commits April 8, 2020 15:47
Use `packaging.version.parse`, not `distutils.version.LooseVersion`.
This allows more complex versions (e.g. with "dev" or "rc") to
be compared properly.
Replace "daily" with "dev" in NxSDK version strings, so that
version comparisons work properly for NxSDK daily builds.

We also enable the following tests, which are unrelated to the
NxSDK 0.9.5 support, but have been fixed by recent commits:
- `test_negative_base` on Loihi
- `test_conv2d_weights[True]`, since CxBase >= 256 support
  is added in cf5411c.
- `test_input_node_precompute`

Co-authored-by: Eric Hunsberger <eric.hunsberger@appliedbrainresearch.com>
@drasmuss drasmuss merged commit d9416e0 into master Apr 9, 2020
@drasmuss drasmuss deleted the nxsdk-091 branch April 9, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants