MT-59 Refactor the CLI to use plain argparse

[lebrice]

• Adds unit tests to establish the current CLI behaviour in regression files:
  • "--help" output
  • ouput when running invalid commands
  • output when running commands that don't involve ssh'ing to the Mila cluster.
• Refactors the CLI to be in plain argparse
  • Uglier for sure, but more transparent, easier to customize and extend to support more arguments (e.g. --cluster=narval option for mila code, etc)
  • Remove coleo as a dependency
  • Add typing_extensions as a dependency
• Updates the regression files to reflect changes:
  • Before running mila or mila serve without choosing a subcommand would print nothing. Subcommands are now required.

[breuleux]

You also need to add the arguments for forward, because standard_server calls forward at the end. These arguments should then be explicitly added to the call to forward (coleo didn't need to do that because it collects arguments recursively in the call tree, so any function that calls forward automatically gains --port).

[lebrice]

How can I be still missing an argument if the help strings are the same as before?

[satyaog]

This changes a lot of things. Could we target a dev branch instead until we add proper testing to the current version of the code?

[lebrice]

This changes a lot of things. Could we target a dev branch instead until we add proper testing to the current version of the code?

I'm not against the idea, but in my opinion, the main branch on this repo is the "latest/unstable" version, while the released versions on PyPI are considered stable.

Also, +1 on adding more tests. One good thing about this here is that it's 100% backward compatible with the previous CLI. The actual commands (mila code, mila serve notebook etc) aren't yet called in tests though.

[satyaog]

If we can make sure that there's no regression, by making sure that in the end the args are correctly passed to the target function, it's seams perfectly right to put this in the master branch. But otherwise I feel like this is a major refactor that might not bring anything yet to the users so could be put in a dev branch until we iron out the tests? Maybe just in case a major additions could to be brought to the users before this is confirmed to not bring any regression?

[lebrice]

I suggest we make another PR to add the proper mock functions and such for testing before the next release.

I doubt there's anyone actually installing milatools from source, so even though this seems pretty safe (as far as I can tell), in the worst case, it shouldn't have that big an impact on the users if this PR gets merged before all the tests have been added.

Edit: another point: Merging this PR (either in a dev branch or in master) will probably make it simpler to write the tests for the next testing PR, since we'll have access to all the function arguments up-front in the args namespace, and we could potentially mock out the actual functions more easily.

Another unrelated idea, but if we wanted to, we could ask for some beta-testers (ish) on the #mila-cluster channel, saying something like: "We're currently refactoring stuff in milatools, if some of you would be interested in helping us, please install the repo from source with pip install git+https://www.github.com/mila-iqia/milatools"

[satyaog]

I don't have any strong objections, but maybe we should merge #55 before this one to not delay it's release further (in a quick new version) as it seams to answer a user issue (contrary to this one, in the short term at least)? Merging this PR in master could also potentially delay other quick fixes answering community concerns if they are raise while making sure everything is fine?

Else, even if there's no foreseeable big impacts for the user, from a philosophical point of view I see more value in adding tests before modifying code, when possible, to prevent unnoticed regressions (unless we're sure that there will be no regression, in which case modifying the code first to ease the addition of tests sounds nice). If that work is too big, I suppose it's ok to add tests afterwards.

In any cases, I'll let you take the final decision, as to merge this now in dev or in master, with or without tests.

Asking for beta testers sounds like a great idea (would also work with pip install "git+https://www.github.com/mila-iqia/milatools@beta_or_whatever")!