Test ROS3 streaming in CI and add ROS3 tutorial #1393

rly · 2021-08-11T19:00:00Z

Motivation

Resolve #1392.

How to test the behavior?

python test.py -r

Checklist

Did you update CHANGELOG.md with your changes?
Have you checked our Contributing document?
Have you ensured the PR clearly describes the problem and the solution?
Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
Have you checked to ensure that there aren't other open Pull Requests for the same change?
Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

codecov · 2021-08-11T20:16:26Z

Codecov Report

Merging #1393 (772236b) into dev (c0ad3e7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #1393   +/-   ##
=======================================
  Coverage   76.78%   76.78%           
=======================================
  Files          37       37           
  Lines        2731     2731           
  Branches      455      455           
=======================================
  Hits         2097     2097           
  Misses        552      552           
  Partials       82       82

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3760038...772236b. Read the comment docs.

rly · 2021-08-11T22:17:20Z

I added a ROS3 streaming test to the CI. The test pulls a small file from DANDI. Please review the test to make sure it makes sense. I hope this doesn't put too much of a burden on DANDI.

The CI runs on CircleCI (linux) on every push and nightly, and it runs on Azure (Windows, MacOS) nightly.

tests/integration/ros3/test_ros3.py

oruebel · 2021-08-11T22:58:44Z

tests/integration/ros3/test_ros3.py

+        filepath = 'sub-anm372795/sub-anm372795_ses-20170718.nwb'
+        s3_path = get_dandi_s3_url(dandiset_id, filepath)


Rather than using an actual user asset, I think it would make sense for DANDI to have an S3 bucket with small test files. This is both to make this easier to control but also because running tests against actual data will throw off usage stats for DANDI.

Having an S3 test bucket on DANDI would also allow testing against NWB files with different versions (as part of a nightly test).

The test now points to https://dandiarchive.s3.amazonaws.com/ros3test.nwb assuming that @yarikoptic or someone else on the DANDI team can host that.

Assuming this is an easy thing for the DANDI team to do, I'd like to hold the release until tomorrow so that we can get this PR in. @oruebel would that be all right?

which file we should host? I would prefer if you provide one so it is not encumbered by any possible fiasco with user wishes/license/suboptimal nwb version etc. Just create one (or some) you would like to see representative of the features you would gain with ROS3 (not fetching "large" data arrays etc) and share and we will gladly host it/them

@rly - nothing is stable till you publish. but once published then you should expect some stability.

regarding redirect, i'm 90% sure the hdf5 library doesn't support a redirect.

That's a good point. Now that we have published datasets, maybe we should use one of those.

filed HDFGroup/hdf5#905

After a dataset is published, can I still add files to it, or is it locked forever?

you can think of it as software releases. you can add, but the published version contains only the assets it was published with.

rly · 2021-08-11T23:18:18Z

@oruebel I added a short tutorial on how to do streaming. Feel free to add to it.

Fix #1367.

oruebel · 2021-08-12T01:50:51Z

I added a short tutorial on how to do streaming. Feel free to add to it.

Thanks! Looks good. I created a thumbnail for the tutorial.

oruebel · 2021-08-12T01:52:58Z

docs/gallery/advanced_io/streaming.py

+#   with NWBHDF5IO(s3_path, mode='r', load_namespaces=True, driver='ros3') as io:
+#       nwbfile = io.read()


Is there a reason for using a context-manager here on read. I'm just wondering since we normally tend to use contexts only on write.

That is a good question. I had always followed the rule of thumb to use a context manager when opening a file to be safe. It should not make a difference in virtually all cases - the file will be closed on garbage collection and having the file open doesn't stop other processes from reading or modifying the file. However, if someone is using Windows and going to delete the file before garbage collection (or thinks another process might delete it), then the file must be explicitly closed before it can be deleted. So I think to be safe, it is ever so slightly better to use a context manager.

import h5py import os filename = 'nwbfile.nwb' f = h5py.File(filename, 'r') os.remove(filename) # fails on windows

See also:
https://stackoverflow.com/questions/56149237/do-i-need-to-manually-close-a-hdf5-file
https://stackoverflow.com/questions/7395542/is-explicitly-closing-files-important/7395906#7395906

…pynwb into ci/ros3

src/pynwb/utils/dandi.py

* rmv s3 utils * rmv corresponding tests * update tutorial to use latest dandi tools instead

bendichter · 2021-08-12T21:22:38Z

@rly you used functions that I wrote a while back before these features were exposed in the dandi python package. Now that those tools are available, it is much simpler to use them. I pushed the change. What do you think?

rly · 2021-08-12T22:53:13Z

@bendichter This looks good, thanks!

oruebel

Looks good. Just for record-keeping, can we add a note in the release notes that there is now a DANDI asset for nightly tests with a link. Not that this is technically a feature of PyNWB but it would be nice to have a record of it.

rly · 2021-08-13T00:01:39Z

Looks good. Just for record-keeping, can we add a note in the release notes that there is now a DANDI asset for nightly tests with a link. Not that this is technically a feature of PyNWB but it would be nice to have a record of it.

Updated now

…pynwb into ci/ros3

rly added 11 commits August 11, 2021 11:57

Test ROS3 CI

21258a7

Run the new job

77d6743

Fixes

83cd67d

Try bypassing tox+conda

7bdcaf8

Use h5py>=3.2

ef222f5

Second try

c0201ec

Different approach, pull out ros3 requirements

4b054a1

Testing

4f790b2

Conda env fix

be5bca7

CI fix

bfd0ceb

CI fix

d245646

rly added 10 commits August 11, 2021 13:23

Finish CircleCI config, set up Azure

04208e2

Fix Azure config

65be04f

Add conda to Azure CI system path

22f117f

Add OS-specific commands to Azure

f530a48

Activate conda env

2879e55

Fix Azure

9d27237

Fix Azure

563b9ee

Fix Windows Azure

4bc1331

Fix typo

60c409b

Remove ROS3 test on Windows/MacOS on every PR

5d39042

rly marked this pull request as ready for review August 11, 2021 22:14

rly requested review from oruebel and bendichter August 11, 2021 22:14

oruebel reviewed Aug 11, 2021

View reviewed changes

tests/integration/ros3/test_ros3.py Outdated Show resolved Hide resolved

oruebel reviewed Aug 11, 2021

View reviewed changes

Update changelog, add basic tutorial

673727f

rly and others added 3 commits August 11, 2021 16:56

Add testing for dandi utils

b7ae928

Use a test file from dandi s3 instead of actual data

7effced

add thumbnail for streaming tutorial

226313c

oruebel reviewed Aug 12, 2021

View reviewed changes

rly added 2 commits August 12, 2021 12:28

Use NWB test dandiset

b032b6e

Merge branch 'ci/ros3' of https://github.com/NeurodataWithoutBorders/…

656227c

…pynwb into ci/ros3

bendichter reviewed Aug 12, 2021

View reviewed changes

src/pynwb/utils/dandi.py Outdated Show resolved Hide resolved

update to use the latest dandi tools

1448b4e

* rmv s3 utils * rmv corresponding tests * update tutorial to use latest dandi tools instead

rly added 5 commits August 12, 2021 16:11

Cleanup, remove references to requests package

1677f91

Update docs

781277d

Update docs

c305176

Update docs

1684074

Fix docs

5447927

bendichter previously approved these changes Aug 12, 2021

View reviewed changes

rly requested a review from oruebel August 12, 2021 23:35

rly dismissed bendichter’s stale review via c0b21c3 August 12, 2021 23:36

Clean up ref to removed file

c0b21c3

oruebel previously approved these changes Aug 12, 2021

View reviewed changes

Update changelog

57cf80b

Merge branch 'ci/ros3' of https://github.com/NeurodataWithoutBorders/…

772236b

…pynwb into ci/ros3

rly dismissed oruebel’s stale review via 772236b August 13, 2021 00:02

rly requested a review from oruebel August 13, 2021 00:02

oruebel approved these changes Aug 13, 2021

View reviewed changes

rly merged commit 7651e9d into dev Aug 13, 2021

rly deleted the ci/ros3 branch August 13, 2021 03:24

oruebel mentioned this pull request Sep 10, 2021

Create short tutorial on data streaming with ros3 #1367

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test ROS3 streaming in CI and add ROS3 tutorial #1393

Test ROS3 streaming in CI and add ROS3 tutorial #1393

rly commented Aug 11, 2021 •

edited

Loading

codecov bot commented Aug 11, 2021 •

edited

Loading

rly commented Aug 11, 2021

oruebel Aug 11, 2021

oruebel Aug 11, 2021

rly Aug 12, 2021

rly Aug 12, 2021

yarikoptic Aug 12, 2021

satra Aug 12, 2021

bendichter Aug 12, 2021

yarikoptic Aug 12, 2021

rly Aug 12, 2021

satra Aug 12, 2021

rly commented Aug 11, 2021

oruebel commented Aug 12, 2021

oruebel Aug 12, 2021

rly Aug 12, 2021

bendichter commented Aug 12, 2021

rly commented Aug 12, 2021

oruebel left a comment

rly commented Aug 13, 2021

		filepath = 'sub-anm372795/sub-anm372795_ses-20170718.nwb'
		s3_path = get_dandi_s3_url(dandiset_id, filepath)

		# with NWBHDF5IO(s3_path, mode='r', load_namespaces=True, driver='ros3') as io:
		# nwbfile = io.read()

Test ROS3 streaming in CI and add ROS3 tutorial #1393

Test ROS3 streaming in CI and add ROS3 tutorial #1393

Conversation

rly commented Aug 11, 2021 • edited Loading

Motivation

How to test the behavior?

Checklist

codecov bot commented Aug 11, 2021 • edited Loading

Codecov Report

rly commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rly commented Aug 11, 2021

oruebel commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendichter commented Aug 12, 2021

rly commented Aug 12, 2021

oruebel left a comment

Choose a reason for hiding this comment

rly commented Aug 13, 2021

rly commented Aug 11, 2021 •

edited

Loading

codecov bot commented Aug 11, 2021 •

edited

Loading