Comprehensive installation and build automation#26
Conversation
hinerm
left a comment
hinerm
left a comment
There was a problem hiding this comment.
I haven't tested this but there were some immediate flags that require attention.
There was a problem hiding this comment.
This file looks like it was auto-generated when updating the auto-translated code. We don't need it.
doc/installation/QUICK_START.md
Outdated
| ### One-Click Installation | ||
|
|
||
| ```bash | ||
| cd tme-quant-napari-curvealign |
There was a problem hiding this comment.
this path doesn't exist. I see it referred to in other files as well like install.sh
There was a problem hiding this comment.
My bad, this was originally written in the napari-curvealign branch, I will need to fix that
doc/installation/QUICK_START.md
Outdated
|
|
||
| The CurveAlign widget will appear in: **Plugins → napari-curvealign → CurveAlign Widget** | ||
|
|
||
| For detailed instructions, see [NAPARI_PLUGIN_INSTALLATION.md](NAPARI_PLUGIN_INSTALLATION.md) |
There was a problem hiding this comment.
Looks like this is just entangled with the napari branch.
There was a problem hiding this comment.
I'll also fix that and make sure the installation automation is independent from those modules.
doc/installation/QUICK_START.md
Outdated
There was a problem hiding this comment.
for the quick_start and setup guide, and maybe also the docker readme, these need to be reconciled with the base README
There was a problem hiding this comment.
Agreed. Note that I did not test the Docker stuff, nor read the quick start, setup guide, docker readme, or other non-main-README docs in my review just now. Here's what my Claude Sonnet 4.5 told me when I asked it to check the documentation for outdatedness/errors:
Claude's report
Issues Found
- QUICK_START.md - Completely Outdated
Problems:
- Still references conda/mamba/Miniforge (lines 24, 26, 29, 36)
- Instructions say to run conda activate tme-quant and napari (lines 36-38)
- Mentions the install script will "Install Miniforge if conda/mamba are not available" (line 24)
- Says "Fail immediately if the tme-quant conda environment already exists" (line 25)
- Discusses creating conda environments (line 29)
Reality:
- The project now uses uv exclusively
- The install script requires uv to be pre-installed
- No conda environment is created anymore
- Users should run uv run napari not conda activate tme-quant; napari
- SETUP_GUIDE.md - Extensively Outdated
Problems:
- Entire "Step 1: Install conda (or Miniforge with Mamba)" section (lines 25-43) is obsolete
- "Step 2: Create Python Environment" (lines 45-54) uses conda/mamba commands
- Manual installation steps don't match the uv-based approach
- Lines 48-53 show creating conda env with mamba
- Line 169 shows conda activate tme-quant
- Troubleshooting section (lines 145-163) discusses conda environments
Reality:
- No conda/mamba installation needed
- Users need uv instead
- Python environment is managed by uv automatically with uv sync
- Docker Configuration - Uses Outdated Stack
Problems:
- docker/Dockerfile uses continuumio/miniconda3:latest as base (line 3)
- Creates conda environment (line 46)
- docker/README.md line 49 says "Uses Python 3.11 in a conda environment"
- Dockerfile doesn't use uv at all
Reality:
- Should use a Python base image or build with uv
- Docker setup doesn't align with the main installation approach
- CI Workflow - Doesn't Use uv
Problems:
- .github/workflows/ci.yml uses pip directly (lines 28-31)
- Uses --break-system-packages flag which is not necessary with uv
- Installs pytest and ruff separately instead of using dependency groups
Reality:
- Should use uv sync and uv run pytest
- The Makefile already uses uv run pytest -v (line 30)
- Makefile - Missing install targets
Problems:
- Lines 11-12 mention make install and make install-dev but these targets don't exist
- Only has setup, test, check, and clean targets
- README.md - Minor Issues
Problems:
- Line 72 says to install pyqt from conda-forge, but project uses PyQt6 from PyPI via uv
- Troubleshooting references conda-forge
Reality:
- Qt dependencies are in pyproject.toml and installed via uv
- install.sh Script - Correct but Could Be Clearer
The script correctly requires uv upfront, but the prerequisite messages could be clearer about what uv does.
Recommendations
High Priority (Critical for Users)
- Rewrite QUICK_START.md
- Remove all conda/mamba references
- Update to reflect uv-based workflow
- Simplify to match actual install.sh behavior
- Update usage instructions to uv run napari
- Rewrite SETUP_GUIDE.md
- Remove conda/mamba installation section
- Add uv installation section
- Update manual installation steps to use uv commands
- Update activation section (no conda activate needed)
- Revise troubleshooting to remove conda references
- Update CI Workflow
- Replace pip commands with uv
- Use uv sync for installation
- Use uv run pytest for testing
- Remove --break-system-packages flag
Medium Priority (For Completeness)
- Update Docker Files
- Modernize Dockerfile to use Python base image
- Install and use uv instead of conda
- Update docker/README.md to reflect changes
- Consider whether Docker is even needed now that uv handles dependencies
- Update README.md
- Remove conda-forge references from troubleshooting
- Clarify that Qt is installed via uv automatically
- Add Makefile Targets
- Either add the missing install and install-dev targets or remove them from help text
- Suggested implementation:
install: uv sync install-dev: uv sync --extra curvelops --group devLow Priority (Nice to Have)
- Update pyproject.toml
- Consider adding a [project.scripts] section for napari-curvealign if you want a CLI entry point
- Clarify Python Version Requirements
- pyproject.toml says >=3.11 (line 10)
- CI tests 3.11, 3.12, and 3.13 (line 18)
- Documentation should be consistent about which versions are supported
Summary
The main issue is that the migration from conda/mamba to uv is incomplete. While the core installation script (bin/install.sh) and Makefile have been updated, the user-facing documentation (QUICK_START.md and SETUP_GUIDE.md) still describes the old conda-based workflow. This creates confusion for users who will follow outdated instructions.
The CI workflow also hasn't been updated to use uv, creating a discrepancy between how developers test locally (with make test using uv) and how CI runs (with pip).
@Phil-Dua Would you be willing to update the documentation along these lines? It would be a good way to learn about uv, as long as you take the time to read and adjust the documentation in cooperation with the AI.
|
The references are now fixed to make sure installation-automation's independence. |
This commit adds installation automation infrastructure that is agnostic of the specific library implementation (Python API or napari plugin). These changes can be merged to main at any time as they are independent of specific API or plugin implementations.
- Replace all 'tme-quant-napari-curvealign' references with 'tme-quant' - Clarify FFTW/CurveLab path requirements in documentation - Remove broken references to non-existent NAPARI_PLUGIN_INSTALLATION.md - Update docker-compose.yml with clearer path documentation - Make all path references relative to tme-quant directory - Ensure installation scripts are independent from external module assumptions
hinerm
force-pushed
the
installation-automation
branch
from
2ad1e47 to
5f14344
Compare
The CurveLab makefile.opt hardcodes an FFTW_DIR path which must be overridden to point to our local installation. Also removes unnecessary env variables that are not used
Building python libraries requires PIC for static linking on x86-64 architectures.
Otherwise ! is picked up on ubuntu
Fix the package name and report if we have curvelets or not
Not applicable or desired for this work. Documentation should be user and developer focused. Records like this should be in git history or in a separate, dedicated location.
hinerm
force-pushed
the
installation-automation
branch
from
df0f0d0 to
1cd4be2
Compare
There was a problem hiding this comment.
@Phil-Dua The given instructions did not work for me (Ubuntu 24, WSL on Windows). I added some commits generally cleaning up the SETUP_GUIDE.md with instructions that I believe will work in general (and worked for me). Please look through them and make sure they make sense to you.
Then I would like you to do some revision of the QUICK_START.md guide and setup scripts. I thought it might be easiest to lay out action items here:
- Remove all the scripts in
\binexceptinstall.sh. They look redundant/unnecessary. The goal of theinstall.shscript is to create a conda environment that encapsulates our installation, so that all a user has to do is "conda activate [env]". This doesn't necessitate script support. And since installation/building usually is a one-time cost we don't need support scripts to configure the environment, etc.. - The
install.shscript (andQUICK_START.mdby extension) should really just be automating the steps in theSETUP_GUIDE.md. Right now, install.sh goes through virtual environments and the SETUP_GUIDE lists conda first. I would like to commit to conda. - Similarly, the
SETUP_GUIDE.mdshould suggest best practices that are compatible with theinstall.shscript - for example, downloading/building FFTW in a../utilsdirectory instead of~/opt. - Also I would like to switch to Mamba. install instructions here (very similar - and less hard-coding to get a platform-appropriate install).
- When using the
install.sh, all that should be required of the user is 1) clone this repo and 2) download CurveLab. The script should handle:- Install Miniforge if conda/mamba aren't available
- Fail fast if the conda environment already exists
- Download FFTW automatically via curl to the target directory (e.g.
../utils) if not already present - Detect which CurveLab version is present in
../utilsand fail with instructions to download CurveLab if not found - Compile both FFTW and CurveLab (installing build tools if needed). And make sure to use PIC when compiling FFTW.
- Create and activate the conda (mamba) environment
- Do the installation (with curvelops)
- Run the verification script (note that I updated this in the
SETUP_GUIDE) - Report the created conda environment and how to activate it
Basically we should only do installations without curvelops if the user knows what they are doing, manually - not with the install script.
I think virtually all these pieces are here, they just need a bit of tweaking.
For the time being I'm not worried about validating the docker instructions/builds right now, or Windows without unix-style environments.
Let me know if this makes sense. If you have any questions at all, we can get on a call and discuss!
|
Regarding mamba: I suggest using pixi instead. Like uv and micromamba, it's a standalone binary that is very easy to bootstrap onto a CI setup and just start using. 👍 |
|
This PR will close #18 |
Per Phil-Dua requirements: - Remove bin scripts except install.sh (deleted activate_env.sh, activate.sh, setup_curvelops_env.sh, setup.sh) - Commit to conda: install.sh uses mamba/conda only (no venv) - Switch to Mamba: prefer mamba when available; install Miniforge if neither conda nor mamba present (platform-appropriate detection) - User prerequisites: 1) clone repo, 2) download CurveLab to ../utils - install.sh now handles: - Miniforge install when conda/mamba unavailable - Fail fast if tme-quant env already exists - Auto-download FFTW via curl to ../utils if not present - Detect CurveLab in ../utils; fail with instructions if missing - Build FFTW (with PIC) and CurveLab; install build tools if needed - Create conda env, install with curvelops - Run verification script - Report env name and activation command - SETUP_GUIDE.md: recommend ../utils for FFTW (not ~/opt); Miniforge/Mamba instructions; conda-only; PIC for FFTW - QUICK_START.md: simplified to clone + CurveLab + bash install.sh Install script always uses curvelops; no-curvelops path is manual only. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Revision complete. I’ve implemented the requested changes:
Ready for review when you have time. |
The test command (and therefore `[ ... ]`) has the `-a` flag for "and", and the `-o` flag for "or", which let the whole expression be evaluated together.
The config.guess and config.sub are too old for modern macOS support. This new logic downloads the latest from the official repository. It's a patch to get things working for now; we should tweak it later to pin to a specific known-to-work version hash, for reproducibility.
There is a source-controlled file in there.
And simplify dependencies list to use pyproject.toml directly. It's far less code, faster, and does not intrude on the user's command line environment the way mamba/conda/miniforge does.
And remove extraneous build targets.
ctrueden
force-pushed
the
installation-automation
branch
from
a3b2697 to
1cef4d9
Compare
The curvelops project has build-system dependency case logic for building with Python <=3.10 versus >=3.11; setting min to 3.11 avoids the need for older build system components.
Bringing them up to date with the new uv-based process.
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
I fixed up this branch to work (mostly) on macOS, as well as greatly reduce the complexity of installation by leveraging the state-of-the-art Python ecosystem tool uv. I don't think we need pixi after all; it seems like all the prerequisites for building from source are available from PyPI now (ninja, cmake, etc.). But if I'm wrong, we could still switch to using pixi instead.
In my view, this PR could be merged any time, as it is a big step forward from what's currently on the main branch. However, there is still some documentation skew, and other imperfections that should be addressed in a timely manner. I defer to @hinerm on how much further he wants to see this branch polished before merging.
| try: | ||
| import pycurvelets as pc | ||
| print(' ✓ pycurvelets') | ||
| print(f' HAS_CURVELETS = {pc.HAS_CURVELETS}') |
There was a problem hiding this comment.
Should validation check that HAS_CURVELETS is actually True here? Right now, it passes validation even if that constant is valued at False. Which I know because on my macOS system, I still haven't succeeded in getting it to return True... 😓
There was a problem hiding this comment.
I’ve updated the validation so it fails when HAS_CURVELETS is False instead of reporting success. The script now exits with a clear error and suggests checking FFTW/FDCT when the curvelet backend isn’t functional. Change is in commit bd70d37 on the installation-automation branch.
|
Pair coded with Curtis and now we are up to date with installation files and pyproject.toml. The next steps are fixing the documentations (removing conda references, etc.) |
|
Remaining work to be done is described by this comment above. |
Align all installation docs with the current setup: uv (not conda), bin/install.sh as the single install script, and pyproject.toml dependencies. Changes: - README.md: Add uv to prerequisites, clarify quick start (uv sync + uv run napari), link to QUICK_START for curvelet setup, update Qt troubleshooting for PyQt6 - doc/installation/QUICK_START.md: Replace conda/Miniforge with uv; document bin/install.sh flow; add 'without curvelets' section - doc/installation/SETUP_GUIDE.md: Rewrite manual steps for uv; add pip alternative; remove conda references; update troubleshooting - doc/requirements.txt: Convert to reference format; note pyproject.toml as source of truth; align with current deps - Makefile: Remove non-existent make install/install-dev from help; clarify make setup Co-authored-by: Cursor <cursoragent@cursor.com>
|
I just tested the newest documentations, and the documented installation flow works on my system. The only caveat is that CurveLab must be present in ../utils (or any parallel directory with tme-quant) for the full install; on a new system without it, the script would stop with instructions to download CurveLab. |
Addresses code review feedback from ctrueden. The install script validation now correctly fails when pycurvelets.HAS_CURVELETS is False, instead of reporting success. Adds a clear error message directing users to check FFTW/FDCT when the curvelet backend is not functional. Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses Curtis's questions about making the napari plugin available: - Add napari.manifest entry point to pyproject.toml so napari discovers the plugin (napari-curvealign = napari_curvealign:napari.yaml) - Include napari.yaml in package data for non-editable installs - Add SETUP_GUIDE section 'Making the napari plugin available' explaining napari.yaml + pyproject entry point, and 'Running for testing' - Add troubleshooting for 'Plugin not showing in napari' - Update install script and QUICK_START plugin menu text Co-authored-by: Cursor <cursoragent@cursor.com>
Phil-Dua
force-pushed
the
installation-automation
branch
from
bbc632a to
a03caeb
Compare
What advantages does Docker potentially provide for tme-quant? The thinking was that Docker might ease installation of curvelops. We cannot legally ship an image with prebuilt CurveLab (no redistribution allowed), but we *could* ship a Docker image with prebuilt FFTW and corresponding build tools, such that the user mounts a folder with tme-quant and unpacked CurveLab, runs the Docker container, and it then builds CurveLab inside the container. The end result is a Linux-only CurveLab binary, which can then be used outside the Docker container. For Linux specifically, this might be slightly easier than demanding build-essential et al. be available on the host system. But the flip side is that we then demand Docker. And for macOS and Windows, this approach does not help unless we also run tme-quant from inside the Docker container (the Docker container won't be building macOS nor Windows binaries). And running graphical applications inside Docker requires (last I checked) running an X server on the host. I don't think the tradeoffs are worth it. We can always revive the Docker support later if we decide it has advantages I'm not seeing.
It's already documented better by pyproject.toml.
- Move doc/installation/QUICK_START.md -> doc/INSTALL.md - Move doc/installation/SETUP_GUIDE.md -> doc/DEVELOPMENT.md - Remove doc/installation/ folder - Remove manual install steps; devs use bin/install.sh - Add 'Without curvelops' section with note on limitations - Update README links Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove TMEQ_RUN_CURVELETS; tests detect curvelops via import - Run curvelet tests when curvelops is available, skip otherwise - Update README, DEVELOPMENT.md, and CI workflow
|
I am super happy with this branch now. I think the blocker is @hinerm's pending "changes requested", but he won't be available any time soon to clear the flag, so here goes the merge! |
|
Thanks everyone for all your hard work on this! |
3 participants
Adds installation automation infrastructure that is agnostic of the specific library implementation (Python API or napari plugin).
These changes can be merged to main at any time as they are independent of specific API or plugin implementations.