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

Using datalad to automate OpenNeuro CLI running #29

Merged
merged 13 commits into from
Sep 17, 2023

Conversation

maelleF
Copy link
Contributor

@maelleF maelleF commented Jul 21, 2023

Closes #30

Checklist

  • PR has an interpretable title with a prefix ([ENH], [BUG], [DOC], [INFRA], [MAINT])
  • PR links to Github issue with mention Closes #XXXX
  • Tests pass
  • Code is properly formatted

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@alyssadai alyssadai self-requested a review July 21, 2023 17:14
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

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

Welcome @maelleF, and thanks for your first PR! 😄

This looks very cool so far; I've suggested a few changes to make the script a bit more flexible in preparation for when we dockerize it and it will be automatically called by some other process.

Please feel free to make the changes and then re-request another review!

datalad_get_single_dataset.sh Outdated Show resolved Hide resolved
datalad_get_single_dataset.sh Outdated Show resolved Hide resolved
datalad_get_single_dataset.sh Outdated Show resolved Hide resolved
@alyssadai alyssadai self-requested a review July 21, 2023 18:49
@surchs surchs added the flag:discuss Flag issue that needs to be discussed before it can be implemented. label Aug 30, 2023
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

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

Hi @maelleF! So sorry about my extremely late follow up to your PR, we recently finally got around to implementing a workaround for the bug you found in the bagel-cli while writing this script.

Now, both the Python and the Docker versions of the bagel-cli should work as expected. That said, it would preferable to switch to using the Docker commands here to avoid needing to set up a Python environment in order to use this script.

I have directly suggested code changes below to switch to using the Dockerized bagel-cli. So as to not step on your contribution (🎉), mind having a look and committing the suggestions? This PR should be good to go at that point!

Looking forward to finally merging this!

datalad_get_single_dataset.sh Outdated Show resolved Hide resolved
datalad_get_single_dataset.sh Show resolved Hide resolved
datalad_get_single_dataset.sh Outdated Show resolved Hide resolved
@alyssadai
Copy link
Contributor

Hey @maelleF, not sure if you will have availability to take this on, so in order to close this PR I'll go ahead and address the suggestions. 🙂

@alyssadai alyssadai self-requested a review September 17, 2023 16:36
@alyssadai alyssadai merged commit 435d529 into neurobagel:main Sep 17, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag:discuss Flag issue that needs to be discussed before it can be implemented.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add script to fetch data and run CLI for a given OpenNeuro dataset ID
3 participants