Unify commands and centralize logging

[0x2b3bfa0]

Changes

• Fixes Better logging #22 by using winston as a logging library instead of manually monkey–patching and aliasing console.log and console.error to custom identifiers. Running commands on a TTY will print colorized, human readable logs; other scenarios (like piping) will produce machine-readable JSONL like in the gh command-line tool. Now there is a single exception handler common for all the command-line interface, eliminating all the duplicate code and delegating the exception formatting on the logging library.

• Fixes Unify commands under a single entry point #699 by moving all the separate commands until a single command named cml and setting up aliases for backwards comaptibility.

• Partially fixes (this is not a closing keyword) Add documentation links and helpful descriptions to error messages #606, but needs additional work from the documentation side so we can link it from the error messages.

• Introduces automatic environment variable processing with yargs.env so cml --long-option=value is equivalent to CML_LONG_OPTION=value cml without any maintenance overhead.

• Introduces plugin support, allowing executables on the PATH to be called as subcommands from the new cml command; e.g. calling cml something will try to find and execute cml-something if it's in your path. As in gh extensions, external commands can't override internal ones under any circumstance.

• Removes the default CMD on our container images (was set to cml-runner for no apparent reason) and sets a smart ENTRYPOINT that interprets unknown commands as cml commands. It didn't make too much sense to favor a tool over the rest and have seven similar ways of checking the installed CML version.

• Fixes an appearance of Tests hang forever #694 caused by an infinite loop on pipe-args.js that doesn't break when there is no piped data; fs.readSync throws EAGAIN but it was being silently ignored.

Follow-ups

• Consider using pino instead of winston for logging #709
• Convert yargs builder function to options object on subcommands #708
• Convert remaining console.log and console.error calls to logger calls #710

---

Note: this time I am erring on the opposite side and packing many loosely related changes together, because shipping them separately would involve lots of intermediate commits with useless, hand-tailored workarounds to keep everything working until the next one.

[DavidGOrtega]

Very nice!!!! 🚀

I think that we could do another iteration.

• Args mostly repeats --repo --token etc...
• I would put everything under CML.js exporting the parameters

What do you think?

[DavidGOrtega]

Also, winston should cover every log, we do not have to it here but would be awesome if we do not forget a technical debt ticket

[0x2b3bfa0]

Also, winston should cover every log, we do not have to it here but would be awesome if we do not forget a technical debt ticket

Yes, #22 states that we should «change every console.log into proper logger calls», but I have preserved some of them on the CLI part to output clean strings like publish asset addresses. Maybe we can create a new output log level to print raw lines. 🤔

[0x2b3bfa0]

Args mostly repeats --repo --token etc...

Yes, I'm leaving that for another pull request, but we could move all the common arguments to the top level cml command.

[0x2b3bfa0]

I would put everything under CML.js exporting the parameters

I would rather separate the command–line interface from cml.js as much as possible, so we have individual files for the commands/subcommands with only the user-facing API, and then all the business logic in the src directory. The old cml-pr command is a good example of this approach.

[DavidGOrtega]

I would rather separate the command–line interface from cml.js as much as possible, so we have individual files for the commands/subcommands

Thats what I say, ideally we just only need one cli file if we expose the ARGS the CML.js module.
have a look 🙂

[0x2b3bfa0]

Thats what I say, ideally we just only need one cli file if we expose the ARGS the CML.js module.
have a look 🙂

Ooops! My bad, I thought you mean src/cml.js and not bin/cml.js 🤦

Yes, we could move all the commands to the latter instead of having separate files, but I still would prefer to keep separate files for each subcommand; it feels cleaner to me. Nevertheless, we could extract the common options to bin/cml.js as you suggested and keep these files to a bare minimum.

[casperdcl]

would've been better to split into separate PRs but whatever

[casperdcl]

perhaps something like this?

Suggested change

	CMD ["cml"]
	ENTRYPOINT ["/usr/bin/cml"]
	CMD ["runner"]

[0x2b3bfa0]

Absolutely for using ENTRYPOINT as the entry point. 💯 Absolutely not for using runner as the default command; it doesn't make any sense (to my mind) and that behavior is not even documented.

[0x2b3bfa0]

Suggested change

	CMD ["cml"]
	ENTRYPOINT ["/usr/bin/cml"]

[casperdcl]

sure but one thing to note is pple who were overriding CMD with e.g. /bin/bash will now get an error. They'll have to switch to using --entrypoint /bin/bash instead.

[casperdcl]

https://docs.github.com/en/actions/creating-actions/dockerfile-support-for-github-actions#entrypoint

[0x2b3bfa0]

docker run iterativeai/cml cml publish # CMD
docker run iterativeai/cml publish # ENTRYPOINT

[DavidGOrtega]

I fiind this better:

docker run iterativeai/cml cml publish # CMD

than this:

docker exec myimage sh -c nvidia-smi

[0x2b3bfa0]

Why not have the best of both worlds?

docker run iterativeai/cml publish
docker run iterativeai/cml nvidia-smi

[0x2b3bfa0]

Applied with a536f07

[DavidGOrtega]

I think that a536f07 is misleading. cml does not support those commands, how is the user going to guess that docker hack? He is not be able to do that with standalone CML.
I do not think is is a good change. What do you think @casperdcl ?

[0x2b3bfa0]

would've been better to split into separate PRs but whatever

I can still split them if you want, but #22 should be addressed after #699 lest writing a dozen of temporary workarounds.

[casperdcl]

not worth the effort now :)

[casperdcl]

@0x2b3bfa0 I'll leave you to open follow up issue(s)/epics

[0x2b3bfa0]

I was doing exactly that 😄

[DavidGOrtega]

To be honest, I do not consider the docker issue fixed to have merged this.
I would have leaved it at least as it was and do a new PR to attack it.