brainglobe / BrainGlobe

General information, resources and dicussions for the BrainGlobe project.
https://brainglobe.info/
9 stars 1 forks source link

Merge bg-atlasapi and bg-atlasgen #43

Closed adamltyson closed 4 months ago

adamltyson commented 7 months ago

Raising this issue to track the idea of merging the atlas generation (really packaging) and atlas API. This could be beneficial to help make sure generated atlases are always compatible with the atlas API and are versioned in the same way. It would also make things simpler for contributors and reduce the number of repos we maintain.

Task list:

willGraham01 commented 7 months ago

Post- or pre- the BrainGlobe v1 rollout (#33)?

Also would we want to preserve the GitHub metrics as with brainreg and cellfinder (in which case I presume we'd merge into bg-atlasapi).

And is this also a good opportunity to rename bg to brainglobe in the name?

adamltyson commented 7 months ago

Also would we want to preserve the GitHub metrics as with brainreg and cellfinder (in which case I presume we'd merge into bg-atlasapi).

Yep

And is this also a good opportunity to rename bg to brainglobe in the name?

Post- or pre- the BrainGlobe v1 rollout (https://github.com/brainglobe/BrainGlobe/issues/33)?

I think after, for two reasons:

  1. Simplifying the release of v1
  2. It might make sense to include this in the work to structure the Python API. I.e. it might be intuitive to have something like from brainglobe import allen_mouse_10um.
willGraham01 commented 7 months ago

We should also include:

when we make these changes.

willGraham01 commented 5 months ago

@adamltyson going to try to come up with a plan to tackle this, but something that I want to double check re:atlasgen.

bg-atlasgen is not available on PyPI currently, because it's meant to be one of our developer facing tools. We encourage install by cloning and then a pip install -e ., and the documentation then says "once your PR is merged, a developer will re-run the atlasgen script" (but I can't find where or what to run as a developer).

So we're in a similar situation to workflows where we are going to end up with a package and a developer tool in the same repository. Based on my understanding of what atlasgen and atlasapi actually do, I think we can do something like the following (in addition to the renaming):

In the merged repo we can even experiment with:

adamltyson commented 5 months ago

All sounds good to me.

"once your PR is merged, a developer will re-run the atlasgen script" (but I can't find where or what to run as a developer).

We need to document this, but it will be one or more of the scripts in bg-atlasgen/atlas_scripts. Typically contributors add a new script, then we run locally & upload to GIN.

pip install brainglobe-atlasapi will have to bundle the brainglobe-atlasgen source code. We can avoid exposing it, but can't avoid distributing it.

We can exclude anything from the manifest arbitrarily if we want though? Not that it's a problem really.

Pull requests with edits to the atlasgen source code trigger additional PR checks to confirm the added atlas is behaving correctly

This may need to be something done in the benchmarking. Generating the atlases is likely to take too long for CI.

Such pull requests, when merged, trigger the atlas upload/update script in a protected workflow.

See above, I think.

willGraham01 commented 4 months ago

@adamltyson @viktorpm I noticed that you both have draft or open PRs against bg-atlasgen. Before I start this merge & rename I just wanted to check if these need review or any help in completion?

I don't want to start the process of merging the two packages and not include any work here that is yet to go in! Happy to be tagged for review if you're just waiting on that.

adamltyson commented 4 months ago

https://github.com/brainglobe/bg-atlasgen/pull/43 and https://github.com/brainglobe/bg-atlasgen/pull/45 are both stalled. As long as some record of this work is preserved, feel free to do whatever you need to progress this.