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

CLI Remove ramp_test_submission, ramp_test_notebook, ramp_convert_notebook, ramp_leaderboard #272

Closed rth closed 3 years ago

rth commented 3 years ago

This is a backport of part of https://github.com/paris-saclay-cds/ramp-workflow/pull/247 (currently in advanced) to master.

Which removes following rampwf.utils.command_line endpoints,

except for ramp_test_submission I imagine are somewhat redundant with rampwf.utils.cli endpoints or not useful? As far as I can tell they are never used in ramp kits or ramp-board?

One concern is that there are deployed ramp events that suggest to use ramp_test_submission instead of ramp-test so we should probably not remove it but rather deprecate (or expose it as an alias to ramp-test if the API is close enough)

For the other 3 endpoints, I'm not sure if we need a deprecation cycle or removing is fine. I imagine users shouldn't have been using those?

@agramfort @glemaitre Please let me know if you have any concern about this. I haven't followed the historical reasons for having both ramp-test and ramp_test_submission.

cc @albertcthomas

TODO:

codecov[bot] commented 3 years ago

Codecov Report

Merging #272 (1da13b6) into master (e12cf36) will increase coverage by 0.20%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
+ Coverage   82.58%   82.78%   +0.20%     
==========================================
  Files         138      137       -1     
  Lines        5059     4677     -382     
==========================================
- Hits         4178     3872     -306     
+ Misses        881      805      -76     
Impacted Files Coverage Δ
rampwf/utils/__init__.py 100.00% <100.00%> (ø)
rampwf/utils/command_line.py 42.42% <100.00%> (-14.98%) :arrow_down:

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 e12cf36...1da13b6. Read the comment docs.

rth commented 3 years ago

Great, thanks for the confirmation!

kegl commented 3 years ago

Don't we use ramp_convert_notebook when we set up the event (to convert the notebook into html and display it on the frontend)? Just a neuron firing from distant memory.

rth commented 3 years ago

As far as I can tell in ramp-board we are now directly using jupyter nbcovert to convert to HTML rather than calling this script.