pybamm-team / pybamm-cookie

A copier template for battery modeling projects using PyBaMM
BSD 3-Clause "New" or "Revised" License
12 stars 3 forks source link

Added `pybamm-cookiecutter` CLI #36

Closed santacodes closed 1 month ago

santacodes commented 1 month ago

Additions

Removals

Usage

To test this, check out this branch and inside the repository and do a local pipx installation on your machine.

git clone https://github.com/santacodes/pybamm-cookiecutter -b cli
cd pybamm-cookiecutter/
pipx install . 

After installation, the CLI will be available systemwide and can be accessed using the pybamm-cookiecutter command. For help references add the -h or --help flag, e.g. pybamm-cookiecutter --help which would prompt with all the available arguments.

Sub-task #26

Saransh-cpp commented 1 month ago

@santacodes could you merge main here?

santacodes commented 1 month ago

@santacodes could you merge main here?

I did merge it, I think the merge commit got hidden within the resolved merge conflicts commit here. Sorry if it didn't show up, I should have probably rebased it but I resolved the merge conflicts locally and pushed them here as they were a bit complicated to resolve and there was no web editor option with this one.

santacodes commented 1 month ago

This is looking good, thanks, @santacodes! Here are some of my suggestions besides the code review that are better provided in text:

  1. Installing and running it displays the message No git tags found in template, using HEAD as ref – this is marked in red, which might prompt users/newcomers to think that something is wrong. Is there a way to disable this message or make it clearer? That said, I don't think this is coming from any of our changes, though.

I think this could be resolved by providing a vcs_ref via the command line or the copier API, I will look into that.

  1. The CLI has a problem where it truncates long sentences, at least on macOS – I noticed that the sentence was terminated before completion as follows: "such as `import p"

Could you please attach a screenshot of some kind it's hard for me to picture it πŸ˜… .

  1. There's no option to go back if I made an error while typing things out – does copier support undo functionality?

I think it does not, looking at the documentation. Maybe I could try to come up with some kind of a hacky way to do it something like recording a particular keystroke for example ctrl+z and then with the cached recorded user inputs, generate the project using the copier API again which would probably continue from where the user left it off. I think that would be something unreliable though, but if we are doing it, this might just be one of the ways to do it.

  1. I think we need better exception handling. If I terminate the program in between by pursuing a KeyboardInterrupt (Command + D on my machine), it prints out "Error caused by an exception:" followed by a long list of metadata that was being used to generate the project, stopped midway. Comparing this with the copier CLI, this is just "Execution stopped by user", which is more elegant. Also, Is there a way to distinguish a keyboard interrupt from an actual error?

I agree with that, I will add better exception handling in the CLI and try to cover most of the endpoints.

  1. Also, in the previous point, a keyboard interrupt midway into creating a project returns an exit code of 0, indicating success, instead of a non-zero exit code to indicate failure – I think it should be 0 for successful completion, 1 for an error in the project generation, and 2 for a keyboard interrupt, and higher for any other non-standard error.

For that, I think we could just wrap the exception handling to KeyBoardInterrupt rather than exit codes.

agriyakhetarpal commented 1 month ago

Yes, letting Python handle the KeyBoardInterrupt will let it use its related exit code, I think. Let's skip the undo functionality for now (or you could try your idea – if the implementation is minimal, that's great).

For the screenshot, here you go:

pybamm-cookiecutter command-line usage example in order to reproduce an error with the truncation of a specific sentence

which indicates that the prompt for the second question was cut off. Maybe there's a limit on the number of characters in that can be configured, or maybe it's a hard limit and we can reword the sentence to make it shorter?

agriyakhetarpal commented 1 month ago

Also, if we were to run copier internally in a subprocess.call() – i.e., sys.exit(subprocess.call(argv)), that should resolve the problem with the exit codes automatically. We'll have to switch to using the copier CLI instead of the API, though.

santacodes commented 1 month ago

Also, if we were to run copier internally in a subprocess.call() – i.e., sys.exit(subprocess.call(argv)), that should resolve the problem with the exit codes automatically. We'll have to switch to using the copier CLI instead of the API, though.

I think it might be a bad idea to use the subprocess library to access the copier CLI, the exit code 0 on KeyboardInterrupt is predefined here in the copier code base even for the CLI. Maybe we should aim for a better exception handling in the pybamm-cookiecutter CLI to cover all the cases.

agriyakhetarpal commented 1 month ago

I would say we should use what brings simpler code to write (and test) – my suggestion is not about handling KeyboardInterrupts, since there are other exceptions to handle. If a subprocess call for us can directly inherit this behaviour from copier, we might as well do so (since copier is not going to deprecate its CLI anytime soon).

santacodes commented 1 month ago

I would say we should use what brings simpler code to write (and test) – my suggestion is not about handling KeyboardInterrupts, since there are other exceptions to handle. If a subprocess call for us can directly inherit this behaviour from copier, we might as well do so (since copier is not going to deprecate its CLI anytime soon).

Umm I think you misunderstood me, you should perhaps look at this copier CLI source code, but in brief, the CLI handles only three exceptions out of which 2 are copier specific errors which are already handled in the Worker class. So basically how all this works is the copier CLI takes in all the arguments from the command line, parses it and passes the command line arguments to a Worker instance, and does the run_copy() method here. In our CLI we are performing the same thing, except we are directly performing the run_copy method of the Worker class like defined here in the source code. So the user prompts which you are concerned about are actually generated by the Worker class and not the copier CLI, which is also why we see the same prompt UI when we use the copier command or access it through the run_copy() command. I just feel we would just increase a layer of abstraction with a subprocess call just for a KeyboardInterrupt exception because right now the pybamm-cookiecutter CLI works like -

pybamm-cookiecutter CLI -> Worker.run_copy while your proposed method would do something like - pybamm-cookiecutter CLI -> copier CLI -> Worker.run_copy

And even the word trimmings you mentioned earlier shouldn't be much of an issue because they only get trimmed after a user answers the prompt if I am not wrong, I tested that locally as well. Overall, I think we should be good with handling the KeyboardInterrupt error in our CLI as that might be the only error we might run into from our side because the Worker class directly handles every other exception case in the core library and not the copier CLI. I hope my explanation made senseπŸ˜…, feel free to correct me haha.

agriyakhetarpal commented 1 month ago

Thanks for the explanation and the cross-links to the code, @santacodes – makes sense to me! Yes, there's no need for extra abstractions, then. Please let us know when this is ready for review again.

Saransh-cpp commented 1 month ago

Could you also add the CI badge in the README - Test template and generated project

agriyakhetarpal commented 1 month ago

Thanks, @santacodes!