-
Notifications
You must be signed in to change notification settings - Fork 113
Describe a DataLad GitHub Action #6931
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
Conversation
5d83d7b
to
1ecf4ee
Compare
Codecov Report
@@ Coverage Diff @@
## master #6931 +/- ##
==========================================
- Coverage 90.02% 88.24% -1.78%
==========================================
Files 354 354
Lines 46273 46352 +79
Branches 6595 0 -6595
==========================================
- Hits 41655 40904 -751
- Misses 4602 5448 +846
+ Partials 16 0 -16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
============= | ||
|
||
Dataset installed at ``${GITHUB_WORKSPACE}/studyforrest-data-phase2``, | ||
``get``'s all the data:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure that getting all the data should be default? E.g. in this demo use case it would be 14GB! (will CI have enough time to fetch it all? ;-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case involves a dedicated small unit testing dataset, but, yes I can see how result in a large download :-).
- get_paths: | ||
- sub-01 | ||
- sub-02 | ||
- stimuli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both install
and get
have --jobs
options. We might want to add here as jobs
and get_jobs
.
Also, for the use-case of "get all data", I think we might better just add also get_data
option to invoke install
with --get-data
and avoid individual get_paths
. Also get_paths
on itself might be "tricky" to ask for all data -- i.e. .
would not be recursive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a pass at this -- please let me know if I did not follow correctly and more changes are needed!
1ecf4ee
to
b3708fb
Compare
b3708fb
to
490920b
Compare
Question (and going to work on this tomorrow) - is there possibly another functionality aside "get"? E.g., for actions I like to design them as small modules, so you could do like: - name: Install Datalad
uses: datalad/datalad-action/install@main # This is the "get" idea
- name: Download Dataset
uses: datalad/datalad-action/download@main - name: Something else?
uses: datalad/datalad-action/??@main Minimally if we can install / get, those should be separate (and the get can use the install) so that's my proposed modification to the above! |
|
||
- uses: datalad/datalad-action@master | ||
with: | ||
datasets: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI (as far as I know) you cannot have nested fields like this.
@yarikoptic can you make me a member of the org or create datalad-action and make me maintainer? I have the install start ready and I'd like to push and test and then work on get. |
Could someone show me an example of the python and/or bash you want for the (core) of the get action? I see both as examples in the discussion so I'm not sure which to use - I was going with Python, and it took me 10 minutes to find (what I think) is the install function in core/local/run.py but I don't see any mention of recursion... so not sure how that comes in. Thanks! |
sorry @vsoch - I am not entirely sure what example you would like? but in general anything "datalad COMMAND [OPTIONS] ARGS" roughly corresponds to import datalad.api as dl
dl.COMMAND(*args, **OPTIONS) and imho In general -- as long as there is action which simplifies installation of datalad, and exposes it to |
@yarikoptic I already figured it out. |
@TheWeX, there is https://github.com/datalad/datalad-action now from @vsoch . Could you have a look at it and give it a shot? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's adjust status (proposed suggestion), take out of draft, and get it merged - we have an implementation by @vsoch so any further discussion etc should happen in its issues.
490920b
to
5bf357a
Compare
Analysis results are not available for those commits View more on Code Climate. |
``datalad`` version to install. Defaults to the latest release. | ||
|
||
``add_datalad_to_path`` | ||
------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why the doc build CI did not run here, but this is invalid RST, and broke it:
Warning, treated as error:
167
/home/runner/work/datalad/datalad/docs/source/design/github_actions.rst:62:Title underline too short.
PR released in |
📝 Documentation
[ci skip]
[appveyor skip]