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

Merge bg-atlasapi and bg-atlasgen #43

Closed
24 tasks done
adamltyson opened this issue Nov 11, 2023 · 7 comments · Fixed by brainglobe/brainglobe-meta#48
Closed
24 tasks done

Merge bg-atlasapi and bg-atlasgen #43

adamltyson opened this issue Nov 11, 2023 · 7 comments · Fixed by brainglobe/brainglobe-meta#48
Assignees
Labels
enhancement New feature or request

Comments

@adamltyson
Copy link
Member

adamltyson commented Nov 11, 2023

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
Copy link

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
Copy link
Member Author

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 (#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
Copy link

We should also include:

when we make these changes.

@willGraham01
Copy link

@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):

  • Merge the source of atlasgen into atlasapi
  • pip install brainglobe-atlasapi will have to bundle the brainglobe-atlasgen source code. We can avoid exposing it, but can't avoid distributing it. It doesn't need to be distributed to users, but there's no way to avoid it whilst also making the package pip install -e-able for compatibility with the steps below.
  • To contribute a new atlas, users follow the same instructions that we currently have, plus some fancier add-ons:
    • Clone the (now merged repository)
    • pip install -e . and edit the brainglobe-atlasgen package in the usual manner
    • Push their branch to GH and open a PR

In the merged repo we can even experiment with:

  • Pull requests with edits to the atlasgen source code trigger additional PR checks to confirm the added atlas is behaving correctly
  • Such pull requests, when merged, trigger the atlas upload/update script in a protected workflow.
  • Pull requests that only touch atlasapi only trigger the those tests to run

@adamltyson
Copy link
Member Author

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
Copy link

@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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants