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.

Sign up for GitHub

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

Jump to bottom

MAINT: Update for latest API #75

Merged
merged 6 commits into from
Oct 14, 2022
Merged

MAINT: Update for latest API #75

merged 6 commits into from
Oct 14, 2022

Conversation

larsoner
Copy link
Collaborator

@larsoner larsoner commented Oct 14, 2022 •
edited by hoechenberger
Loading

Locally I get something that seems to work:

$ python -ui mne_bids_pipeline/tests/run_tests.py --download=1 ds000248
...
🌍 Preparing to download ds000248 …
📁 Traversing directory structure for ds000248 (this can take a while) …
    derivatives 
    │ derivatives/freesurfer 
    │ │ derivatives/freesurfer/subjects 
    │ │ │ derivatives/freesurfer/subjects/fsaverage 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/bem 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/label 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/mri 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/mri/transforms 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/mri.2mm 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/scripts 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/surf 
    │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/label 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/mri 
    │ │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/mri/transforms 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/scripts 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/surf 
    │ │ │ │ │ derivatives/freesurfer/subjects/fsaverage/xhemi/touch 
    │ │ │ derivatives/freesurfer/subjects/morph-maps 
    │ │ │ derivatives/freesurfer/subjects/sub-01 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/bem 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/bem/flash 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/label 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/T1 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/brain 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash 
    │ │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash/parameter_maps 
    │ │ │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash/parameter_maps/fsl_rigid_register.10366 
    │ │ │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash/parameter_maps/fsl_rigid_register.14439 
    │ │ │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash/parameter_maps/fsl_rigid_register.15197 
    │ │ │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/flash/parameter_maps/fsl_rigid_register.9149 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/orig 
    │ │ │ │ │ derivatives/freesurfer/subjects/sub-01/mri/transforms 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/scripts 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/stats 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/surf 
    │ │ │ │ derivatives/freesurfer/subjects/sub-01/touch 
    sub-01 
    │ sub-01/anat 
    │ sub-01/meg 
    sub-emptyroom 
    │ sub-emptyroom/ses-19210819 
    │ │ sub-emptyroom/ses-19210819/meg 
📥 Retrieving up to 1220 files (5 concurrent downloads). 
✅ Finished downloading ds000248.                                                                                                                                                   
                                                                                             
🧠 Please enjoy your brains.

I've tried it half a dozen times and so far no retry limit problem on the traversal. So I think we can leave it for future work to combine the filename traversal (which uses a generator/yield already) with file download queuing.

@hoechenberger feel free to try and merge if you're happy. I'm going to tell MNE-BIDS-Pipeline to use this branch in an upcoming PR, too, so we'll get some feedback there.

Closes #64
Closes #73

@larsoner larsoner mentioned this pull request Oct 14, 2022
Merged
@larsoner
Copy link
Collaborator Author

Made things green in mne-tools/mne-bids#1081

@larsoner
Copy link
Collaborator Author

And in mne-tools/mne-bids#1083

@larsoner
Copy link
Collaborator Author

Download steps appear to be succeeding in mne-tools/mne-bids-pipeline#634 / https://app.circleci.com/pipelines/github/mne-tools/mne-bids-pipeline/3354/workflows/47b0dfe0-90d0-4e3a-9211-492bca84c886

@larsoner larsoner mentioned this pull request Oct 14, 2022
Closed
@hoechenberger
Copy link
Owner

@larsoner I've pushed the following changes

  • some stylistic changes regarding to line breaks / indents (not very important, as I'm planning to move this project to using Black soon anyway, but still)
  • I added type annotations to your new function
  • I've compacted the output during directory traversal:
🌍 Preparing to download ds000248 …
📁 Traversing directories for ds000248 : 1228 entities [00:19, 61.64 entities/s] 
📥 Retrieving up to 1220 files (5 concurrent downloads).

WDYT?

@larsoner
Copy link
Collaborator Author

📁 Traversing directories for ds000248 : 1228 entities [00:19, 61.64 entities/s]

Okay for me! This can take 30-60 sec so as long as there is actually some output to tell people "it's not broken, it's just slowly working" I'm happy

@hoechenberger
Copy link
Owner

Yes, it keeps updating!! See:

Screen.Recording.2022-10-14.at.23.18.04.mov

@hoechenberger
Copy link
Owner

@larsoner Feel free to merge if happy

@larsoner
Copy link
Collaborator Author

The only comment I have with this iteration is that the directories/s is probably more stable -- it's right about 0.5 on my system. entities/sec will naturally vary (much) more based on the "entities per directory", since the rate limiting step here is the "directory fetch" not the "number of files per directory". Worth changing to directories/sec instead?

@larsoner larsoner enabled auto-merge (squash) October 14, 2022 22:12
@larsoner
Copy link
Collaborator Author

Worth changing to directories/sec instead?

Now that it's faster, and seeing how simple it is with the entities/files (and how complicated it would have to be to support directories) -- I don't think it's worth it. I push a commit to speed it up by a factor of ~2 by avoiding the "snapshot check" during file iteration, as we've already validated that the snapshot exists by that point. So I've marked this for merge. Feel free to create a release after this is in @hoechenberger !

@larsoner larsoner merged commit 30556c2 into hoechenberger:main Oct 14, 2022
@larsoner larsoner deleted the iterate branch October 14, 2022 22:17
@larsoner larsoner restored the iterate branch October 14, 2022 22:17
@hoechenberger
Copy link
Owner

Amazing, great work, @larsoner!!!!

hoechenberger added a commit that referenced this pull request Oct 14, 2022
@hoechenberger
Update changelog
e4be8d3 
We forgot to do that in #75
@hoechenberger hoechenberger mentioned this pull request Oct 14, 2022
Merged
hoechenberger added a commit that referenced this pull request Oct 14, 2022
@hoechenberger
Update changelog (#76)
2bc403d 
We forgot to do that in #75
@hoechenberger
Copy link
Owner

@larsoner Do you have any idea why in this workflow:
https://github.com/hoechenberger/openneuro-py/actions/runs/3253382223

the version number is reported as 2022.1.0 and not as the latest one (2022.2.0)? It works for me locally, but it seems that workflow is ignoring the newly-added tag??

@hoechenberger
Copy link
Owner

Oh … not true, even locally I get the old version number! Ouch…

@hoechenberger
Copy link
Owner

Oooohhh… we're not using setuptools-scm here! OUCH :)

@hoechenberger
Copy link
Owner

All good, new release was automatically pushed to PyPI!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenNeuro removing prefix: null for file listings
2 participants
@larsoner @hoechenberger