paris-saclay-cds / ramp-workflow

Toolkit for building predictive workflows on top of pydata (pandas, scikit-learn, pytorch, keras, etc.).

https://paris-saclay-cds.github.io/ramp-docs/

BSD 3-Clause "New" or "Revised" License

68 stars 43 forks source link

Autocompletion for submission names #261

Closed albertcthomas closed 3 years ago

albertcthomas commented 3 years ago

If this can be useful to anyone I am sharing this here.

This was done following click documentation.

As written in the documentation I need to activate the autocompletion for my shell for this to work.

kegl commented 3 years ago

Amazing, thanks!

Could also be done for the data label, examining folders in ./data

@agramfort do you or someone from Saclay wants to approve it?

albertcthomas commented 3 years ago

Could also be done for the data label, examining folders in ./data

Yes in the advanced branch.

albertcthomas commented 3 years ago

Could you please add,

I need to activate the autocompletion for my shell for this to work.

somewhere in the ramp-workflow documentation?

Yes, this could also benefit from some tests, if not the full autocompletion thing, which can be hard to test, maybe the function itself.

albertcthomas commented 3 years ago

I also tried something using the args argument of the get_submissions function so that we could look if ramp-submission-dir is specified and look into this directory. However I don't really know how this path is specified in the command line and trying it lead to several errors that I did not want to deal with to not make a function hard to test too complicated. Besides is there a real use of this option besides on the server running the submissions submitted by the users and in the tests where no autocompletion is needed? It seems I would prefer telling the users to call ramp-test in the root dir where they have the problem.py and the submissions folder. Anyway this is maybe going out of the scope of this PR :)

agramfort commented 3 years ago

LGTM

+1 for merge if CIs are green

albertcthomas commented 3 years ago

Could you please add,

I need to activate the autocompletion for my shell for this to work.

somewhere in the ramp-workflow documentation?

Yes, this could also benefit from some tests, if not the full autocompletion thing, which can be hard to test, maybe the function itself.

Actually, ramp-test is not in the documentation, only ramp_test_submission which does not use click...

codecov[bot] commented 3 years ago

Codecov Report

Merging #261 (6f9e024) into master (db9821f) will increase coverage by 0.38%. The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   81.75%   82.13%   +0.38%     
==========================================
  Files         133      133              
  Lines        4921     4938      +17     
==========================================
+ Hits         4023     4056      +33     
+ Misses        898      882      -16

Impacted Files	Coverage Δ
rampwf/utils/cli/testing.py	`61.53% <75.00%> (+61.53%)`	:arrow_up:
rampwf/utils/cli/tests/test_cli.py	`96.72% <100.00%> (+0.56%)`	:arrow_up:

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 db9821f...6f9e024. Read the comment docs.

agramfort commented 3 years ago

do you have a minute to update the doc? thx

no objection to merge here

albertcthomas commented 3 years ago

do you have a minute to update the doc? thx

I am not sure what the plan is here. Should we just replace ramp_test_submission by ramp-test? Add a ramp-test entry? What was the reason to keep both? Backward compatibility?

agramfort commented 3 years ago

I would just document ramp-test now. ramp_test_submission should ideally be deprecated

albertcthomas commented 3 years ago

I’ll document ramp-test and leave the rest for another PR

albertcthomas commented 3 years ago

ramp-test was in fact already in the master doc (but not in the doc of the last release).

kegl commented 3 years ago

I took care of this already, the old cli is deleted from the advanced branch: https://github.com/paris-saclay-cds/ramp-workflow/blob/advanced/setup.py https://paris-saclay-cds.github.io/ramp-docs/ramp-workflow/advanced/command_line.html

We should merge it into master.

albertcthomas commented 3 years ago

It's already in master (https://github.com/paris-saclay-cds/ramp-workflow/blob/master/doc/command_line.rst). I got confused for not looking at the correct version: the documentation website shows the documentation of the last stable release (0.3.3), not the one of master (0.4).

agramfort commented 3 years ago

Thx @albertcthomas

albertcthomas commented 3 years ago

Thanks for the reviews and comments :)