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

Coexistance of cellfinder+brainreg CLI tool and cellfinder-core workflow #58

Merged
merged 18 commits into from
Dec 21, 2023

Conversation

sfmig
Copy link
Contributor

@sfmig sfmig commented Dec 14, 2023

Context

This repository currently serves a few purposes:

  • Host the cellfinder CLI tool (onwards called 'cellfinder+brainreg CLI tool' for clarity).
  • Host example workflow scripts.
    • These are useful for developers to benchmark (see next point) and for users only as scripts to "copy-paste" and adapt to their own use-cases. Right now there is only onecellfinder-core workflow script.
  • Host benchmarks that time the workflow scripts.
    • These are only useful for developers.

Our understanding is that the cellfinder+brainreg CLI tool will be gradually discontinued (in line with the brainglobe general aim of removing CLI tools) and turned into a workflow script instead. This PR outlines a strategy to get there.

Goal of this PR

With this PR, the brainglobe-workflows PyPI package will ship:

  • the cellfinder+brainreg CLI tool, and
  • the cellfinder_core workflow script.

The cellfinder_core workflow script is only included in the source code, and users are not meant to be "aware" of it (aka, it is not documented in the user docs). Users will use the cellfinder+brainreg CLI tool in the same way as they did before.

Instead, developers are aware of the cellfinder_core workflow script and of the benchmarks defined on top of it (that is, both things are documented in the dev docs).

How do users use this repo?

Users will pip install brainglobe-workflows and use the cellfinder+brainreg CLI tool as usual, with the cellfinder entrypoint.

How do devs use this repo?

Devs will:

  1. git clone this repo
  2. pip install .[dev]
  3. Run benchmarks locally with asv run or run tests with tox/pytest

Three sets of dependencies

To cater for these different audiences, we divided the dependencies in pyproject.toml into three sets:

  • dependencies, the required ones. We propose that these contain the dependencies for the cellfinder+brainreg CLI tool only, for now (see Longer term section).
  • dev, contains the dependencies required by developers to contribute to the repo (run tests, benchmarks, etc).
  • asv_version, contains the dependencies required by asv to be able to run the benchmarks.
    • The idea is that these are the dependencies needed to install the package without the CLI tool.
    • Once the cellfinder+brainreg CLI tool is deprecated, these dependencies will move to the default dependencies section.
    • Note that asv works by creating a new virtual env, installing the software version of a given commit into it, and running the benchmarking functions on it.

Longer term

  • In the short-medium term, we need to ensure PRs don't break the cellfinder+brainreg CLI tool.
  • The idea would be to only update the package in PyPI when the CLI tool is updated.
  • In the longer term, the cellfinder+brainreg CLI tool will be transformed into a workflow script, and we will no longer have the need to publish a package in PyPI of this repo. When that happens all of the dependencies under asv_version would move to the required dependencies section.

Specifics of this PR

  • Restructured package internals (and adapted entry point accordingly)
  • Modified the asv package install command, to install the brainglobe-workflows package with asv_version dependencies only.
  • Updated the README.md to split user and dev goals more clearly.
  • Defined three sets of dependencies in pyproject.toml

Rebase after merging #27

@adamltyson
Copy link
Member

I might have missed something, but is this needed? What used to be called cellfinder-core is now called cellfinder anyway now.

@alessandrofelder
Copy link
Member

We need to somehow distinguish the CLI tool (present in this repo) from benchmarked scripts using the Python tool's API?

@sfmig
Copy link
Contributor Author

sfmig commented Dec 14, 2023

my understanding is that the cellfinder+brainreg CLI tool is called in this repo cellfinder after the migration of the cellfinder repo to the brainglobe-workflows repo. This is in conflict with me previously using cellfinder to define a cellfinder-core workflow.

@adamltyson
Copy link
Member

my understanding is that the cellfinder+brainreg CLI tool is called in this repo cellfinder after the migration of the cellfinder repo to the brainglobe-workflows repo. This is in conflict with me previously using cellfinder to define a cellfinder-core workflow.

This should change soon anyway, but maybe it's good to specify cellfinder_core to distinguish from cellfinder.napari anyway.

@sfmig sfmig changed the title Rename cellfinder workflows to cellfinder core Coexistance of cellfinder+brainreg CLI tool and cellfinder-core workflow Dec 15, 2023
Copy link
Collaborator

@willGraham01 willGraham01 left a comment

Choose a reason for hiding this comment

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

The plan in the description sounds a lot more synthesised than in our Thursday programming session, great job 😄

Some comments on the README but most of them are me just rephrasing things or VSCode markdown lint getting angry. Haven't touched the ASV stuff because it scares me.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@adamltyson
Copy link
Member

In the longer term, the cellfinder+brainreg CLI tool will be transformed into a workflow script

Slight comment, I think we will maintain some CLIs (i.e. just the command line interface) that are heavily used. However, I think we should move most of the code inside these workflows to other packages so that it can be a single workflow script.

@sfmig sfmig force-pushed the smg/rename-cellfinder-core branch from b04a067 to bf19874 Compare December 16, 2023 14:04
Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (24f2592) 81.37% compared to head (9c19237) 81.39%.

Files Patch % Lines
benchmarks/cellfinder_core.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   81.37%   81.39%   +0.02%     
==========================================
  Files          32       32              
  Lines        1584     1586       +2     
==========================================
+ Hits         1289     1291       +2     
  Misses        295      295              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sfmig sfmig marked this pull request as ready for review December 20, 2023 10:32
@sfmig sfmig requested a review from willGraham01 December 20, 2023 11:00
@sfmig sfmig mentioned this pull request Dec 20, 2023
Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Great - happy with this plan.

@alessandrofelder alessandrofelder merged commit 0d99ec4 into main Dec 21, 2023
10 checks passed
@alessandrofelder alessandrofelder deleted the smg/rename-cellfinder-core branch December 21, 2023 09:58
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.

4 participants