Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor CLI demos to be applications #1211

Merged
merged 60 commits into from
May 1, 2024
Merged

Refactor CLI demos to be applications #1211

merged 60 commits into from
May 1, 2024

Conversation

pgunn
Copy link
Member

@pgunn pgunn commented Oct 25, 2023

This is work to refactor the CLI demos.

In exchange for giving up on them being used notebook-style (spyder, VSCode, ..), which this more-or-less entirely does by using argparse and having them take commandline arguments to control their behaviour:

We get the ability for people to use them without modifying them at all. If we continue in this direction, particularly with making all visualisations optional (and controlled by a commandline arg - we DO want to show visualisations by default), and with possible future changes we've been talking about - to have stable and documented output formats from Caiman, people might be able to comfortably run Caiman in headless environments outside of notebooks, again with no code edits (if we offer a good library of demos that let people parameterise the algorithms fully with CLI flags).

In terms of this particular diff, I am not attached to it. I am not sure if these are the best names for the parameters. It didn't take me too long to do it, and I could redo it in a different style without a lot of effort. This is mostly intended as a discussion piece (although if there is strong enthusiasm for exactly this, we might land this).

@EricThomson
Copy link
Contributor

EricThomson commented Oct 25, 2023

After talking, I like the idea of making a very clear separation between "cell based" (Jupyter) demos, and true CLI-based demos like this. It would drastically reduce our violation of DRY which really bugs me with the current setup (and generates an issue every couple of months).

We'd probably want to warn people, give them a release or two lead time about what we are planning (either in the readme, or the non-code central repo we'll be making soon that will contain news and such). Also, I'm not sure of the best way, but we'd probably want something added to docs to the effect of "here's how to run a CLI module" because it is becoming a bit of a lost art.

@pgunn
Copy link
Member Author

pgunn commented Oct 27, 2023

I stashed a copy of the current-form CLI demos here, as per our slack discussion:

https://github.com/flatironinstitute/caiman_use_cases/tree/main/use_cases/old_cli_demos

@pgunn
Copy link
Member Author

pgunn commented Oct 27, 2023

I don't think we need to move that slowly on this; few people read release notes for future changes, and I suspect just making this change for the next release would be the right thing. Noting it as some future change just for the sake of giving notice is too much process.

@EricThomson
Copy link
Contributor

I agree about release notes. I'm thinking we shouldn't release this change without adding the supporting infrastructure/explanation in the docs/readme. This is in effect a breaking change at the demo level. It might be nice to discuss at Community Meeting to let people know (next Community Meeting is at the workshop in two weeks).

What counts as "supporting infrastructure/explanation"? Just off the top of my head: we should mention the division in the readme (and maybe a link to the old ones, and "if you want to run code cells just use Jupyter" or whatever as we discussed), with link to new place in docs about how these cli's work and how to run them.

@pgunn
Copy link
Member Author

pgunn commented Oct 27, 2023

Ouch, I just spotted that we actually never even documented the CLI demos in the first place. They're delivered, but they don't even have a readme of their own, nor are they mentioned in the main readme. I think this diff probably should fix that; ideally not in a "we plan to do this" kind of way, but at least in a "this is what this folder is" kind of way, which is an improvement over the status quo.

@pgunn
Copy link
Member Author

pgunn commented Nov 3, 2023

As I work on this diff, I want all the CLI demos to, as much as possible, have as similar a set of arguments as I can manage, so people can switch from one to the other without a lot of effort and so it feels like people are not learning a number of very different tools.

@pgunn pgunn changed the title Trying out a refactor of the basic CLI demo Refactor CLI demos to be applications Nov 21, 2023
@pgunn pgunn force-pushed the cli_demos_refactor branch from 77bd5e8 to 5677645 Compare February 27, 2024 21:54
Comment on lines 45 to 48
if cfg.configfile:
opts = cnmf.params.CNMFParams(params_from_file=cfg.configfile)
if opts.data['fnames'] is None:
opts.set("data", {"fnames": fnames})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And finally we reach (the start of) one of the highlights of all this work: JSON-based configs for CLI tools.

pgunn added 21 commits April 16, 2024 11:23
pgunn added 4 commits April 16, 2024 11:25
This once was functional, with #52a0c4b1, but became vestigial with a later refactor of cnmf.merging.merge_components()
@pgunn pgunn force-pushed the cli_demos_refactor branch from 96802bf to d87162f Compare April 16, 2024 15:28
@pgunn
Copy link
Member Author

pgunn commented Apr 19, 2024

As part of this diff, I'm cleaning up path code all over the place to try to get the demos to stop dropping files randomly all over the place. This is not the path cleanup that will get us per-run output directories, but it may make it easier to get there down the line.

@pgunn
Copy link
Member Author

pgunn commented Apr 19, 2024

Finding and fixing a lot of weird behaviour across the codebase with this work.

@pgunn
Copy link
Member Author

pgunn commented Apr 26, 2024

This is now almost ready to merge; I've run all the CLI demos with it and they generally behave similarly to how they did before, and I ran through the GUI demos to make sure neither this nor the CNMFParams changes broke them, and along the way I got a lot of code paths across the codebase to stop dropping files in places they don't belong. I didn't fix everything I wanted to, and some of the notebooks/clidemos have other problems that we'll want to pay attention to (I fixed some things on the dev branch, and filed a bug on .mat files being broken). The CLI demos need some more attention in the future to get the graphics handling to be better.

We have a new readme that will be delivered in the demos directory too.

Further in the future there's a lot more files-based cleanup; the next step along that route will be to create an empty directory for runs and to note all the places in the codebase where files are created/read/... but for now I'm happy to reduce the randomness and have better CLI demos.

@pgunn
Copy link
Member Author

pgunn commented May 1, 2024

This is finally ready for merge; we have docs, the demos all run and for the first time since the spyder days, they produce the intended graphical outputs, we've removed some dead code paths and fixed a lot of bugs.

@pgunn pgunn merged commit 6581329 into dev May 1, 2024
3 checks passed
@pgunn pgunn deleted the cli_demos_refactor branch May 3, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants