search

napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.1k stars 410 forks source link

Prototype a conda-based bundle #3555

Closed jaimergp closed 2 years ago

jaimergp commented 2 years ago

Comes from napari/napari#3378

Description

Implements logic discussed in napari/napari#3358 - proof of concept!

Type of change

References

Closes napari/napari#3358

How has this been tested?

Pending tasks

General

Windows

MacOS

Linux

Other tools

Final checklist:

jaimergp commented 2 years ago

I don't get it, I cloned and installed your branch of constructor and napari. But, i get a bunch errors regarding [definitions]:

Hm, weird. It's working fine here in the CI. Are you sure you installed the branch? The workflow would be, roughly:

  1. Create a new conda environment for constructor: conda create -n constructor-dev constructor
  2. Force remove constructor it self: conda activate constructor-dev; conda remove --force constructor
  3. Clone my fork and checkout my branch: git clone git@github.com:jaimergp/constructor; cd constructor; git checkout menuinst+branding
  4. Install it in dev mode: pip install -e .
  5. Go to the napari repo, checkout my branch and run python bundle_conda.py.
jaimergp commented 2 years ago

looks like I need conda-standalone to get anywhere and that's not available on conda-forge for arm64.

We have a build in our napari channel, label bundle_tools.

jaimergp commented 2 years ago

Aaaand signed and notarized! No warnings or anything! Finally! 🥳

image

Check yourself at https://github.com/napari/napari/actions/runs/1707851904

psobolewskiPhD commented 2 years ago

I posted this in zulip, but maybe better here. Install works: just double clicks, no authentication, etc. the app is in ~/Applications and the conda is in ~/Library! But it doesn't launch 😢 Bounces a few times and then disappears from dock 😢

I tried something. I went to the conda: cd ~/Library/napari-0.3.13rc4.dev70/ and then found napari in ./bin I tried running it: ./napari But that gave this error:

Traceback (most recent call last):
  File "/Users/piotrsobolewski/Library/napari-0.4.13rc4.dev70/bin/./napari", line 6, in <module>
    from napari.__main__ import main
  File "/Users/piotrsobolewski/Library/napari-0.4.13rc4.dev70/lib/python3.9/site-packages/napari/__main__.py", line 15, in <module>
    import napari.plugins._npe2 as _npe2
  File "/Users/piotrsobolewski/Library/napari-0.4.13rc4.dev70/lib/python3.9/site-packages/napari/plugins/__init__.py", line 1, in <module>
    from npe2 import PluginManager as _PluginManager
ModuleNotFoundError: No module named 'npe2'

Seems like the same issue that bit me earlier: https://napari.zulipchat.com/#narrow/stream/309872-plugins/topic/.E2.9C.94.20can't.20seem.20to.20get.20plugin.20to.20show.20up/near/268132311

Yup: your branch doesn't have npe2 in requirements here: https://github.com/napari/napari/blob/conda-bundle-poc/setup.cfg

Edit: it's odd that it totally fails to run tho. But I fixed it!!

conda activate ~/Library/napari-0.4.13rc4.dev70
pip install npe2

Edit2: I think that only worked because I had an existing conda in path. trying to use the conda in the condabin asked me about doing init for my shell, I wasn't sure if that would mess with my existing conda so i didn't do it...

jaimergp commented 2 years ago

trying to use the conda in the condabin asked me about doing init for my shell, I wasn't sure if that would mess with my existing conda so i didn't do it..

Good call. It would have. That's why we have the shortcut stuff in place: to do non-invasive, proper conda activations. You can run it from the terminal too: ~/Applications/napari-x.y.z.app/Contents/MacOS/napari-x.y.z.

You are right, we are missing a dependency, but not in our branch directly. It needs to be added to the conda-forge recipe we have been using so far. I also updated the dependencies there.

@sofroniewn -- could we sync a bit before the final release to make sure the conda-forge recipe is accurate?

psobolewskiPhD commented 2 years ago

Let's say a bundle user decided to become a python user... ...would doing the conda init basically init their shell and give them conda et al on the path? Could they then make new envs, etc as they please? Edit: if so, then this napari bundle seems like a really powerful drug!! @jni

jaimergp commented 2 years ago

would doing the conda init basically init their shell and give them conda et al on the path?

Yep, we basically shipping our own custom Miniconda, which happens to contain napari and a few extra deps.

Czaki commented 2 years ago

In PartSeg, I have a particular command-line argument _test, which performs a basic test of the bundle (with importing modules, creating potentially problematic objects, but without showing a window or starting qt event loop).

Maybe it could be a good idea for a testing bundle on CI?

jaimergp commented 2 years ago

That said, it's a scary situation :D We haven't added any safeguards to the "base" env, so if you start modifying the installation you might end up with a non-functional napari. There are ways to make this happen, but I am not sure it's a path we want to pursue right now. It does have some synergies with the plugin isolation work.

tlambert03 commented 2 years ago

You are right, we are missing a dependency, but not in our branch directly. It needs to be added to the conda-forge recipe we have been using so far. I also updated the dependencies there.

when you say "there", what are you referring to, https://github.com/conda-forge/napari-feedstock?

@sofroniewn -- could we sync a bit before the final release to make sure the conda-forge recipe is accurate?

generally, our pattern has been to make sure that all deps are available on conda, and then to update the recipe once the package is on pypi and the auto-build is triggered. for this release here's the diff in setup.cfg from the last release:

diff --git a/setup.cfg b/setup.cfg
index b82145dc5..67419431d 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -21,6 +21,7 @@ classifiers =
     Programming Language :: Python :: 3.7
     Programming Language :: Python :: 3.8
     Programming Language :: Python :: 3.9
+    Programming Language :: Python :: 3.10
     Topic :: Scientific/Engineering
     Topic :: Scientific/Engineering :: Visualization
     Topic :: Scientific/Engineering :: Information Analysis
@@ -42,15 +43,17 @@ install_requires =
     cachey>=0.2.1
     certifi>=2018.1.18
     dask[array]>=2.15.0,!=2.28.0  # https://github.com/napari/napari/issues/1656
-    imageio>=2.5.0
+    imageio>=2.5.0,!=2.11.0
     importlib-metadata>=1.5.0 ; python_version < '3.8'
     jsonschema>=3.2.0
-    magicgui>=0.3.0
+    magicgui>=0.3.3
     napari-console>=0.0.4
     napari-plugin-engine>=0.1.9
     napari-svg>=0.1.4
+    npe2>=0.1.1
     numpy>=1.18.5
     numpydoc>=0.9.2
+    pandas>=1.1.0
     Pillow!=7.1.0,!=7.1.1  # not a direct dependency, but 7.1.0 and 7.1.1 broke imageio
     pint>=0.17
     psutil>=5.0
@@ -58,17 +61,19 @@ install_requires =
     PyYAML>=5.1
     pydantic>=1.8.1
     qtpy>=1.7.0
-    scipy>=1.2.0
-    superqt>=0.2.4
+    scipy>=1.4.0
+    superqt>=0.2.5
     tifffile>=2020.2.16
     typing_extensions
     toolz>=0.10.0
     tqdm>=4.56.0
-    vispy>=0.6.4, !=0.8.0
+    vispy>=0.9.4
     wrapt>=1.11.1

 [options.package_data]
 * = *.pyi
+napari =
+    builtins.yaml

 # for explanation of %(extra)s syntax see:
 # https://github.com/pypa/setuptools/issues/1260#issuecomment-438187625
@@ -95,17 +100,16 @@ testing =
     pytest-faulthandler
     pytest-order
     pytest-qt
-    pytest-timeout
     hypothesis>=6.8.0
-    pandas
     xarray
     fsspec
     zarr
-    scikit-image>=0.18.1
+    matplotlib
+    scikit-image>=0.18.1,!=0.19.0
     pooch>=1.3.0
     semgrep
-    tensorstore>=0.1.10,!=0.1.11
-    torch>=1.7
+    tensorstore>=0.1.13 ; python_version < '3.10'
+    torch>=1.7 ; python_version < '3.10'
     meshzoo
 release = 
     PyGithub>=1.44.1
@@ -115,7 +119,9 @@ dev =
     pre-commit>=2.9.0
     black==20.8b1
     flake8==3.8.4
+    pydantic[dotenv]
     check-manifest>=0.42
+    rich
     %(all)s
     %(testing)s
 build =
@@ -143,15 +149,16 @@ console_scripts =
     napari = napari.__main__:main
 pytest11 =
     napari = napari.utils._testsupport
-
-
+napari.manifest =
+    napari = napari:builtins.yaml
+    
 [flake8]
 # Ignores - https://lintlyci.github.io/Flake8Rules
 # E203  Whitespace before ':'  (sometimes conflicts with black)
 # E501 line too long (84 > 79 characters)  (sometimes too annoying)
 # W503 Line break occurred before a binary operator
 # C901 McCabe complexity test. Would be nice to re-enable, but takes work
-ignore = E203,W503,E501,C901
+ignore = E203,W503,E501,C901,D401
 max-line-length = 79
 max-complexity = 18
 exclude = _vendor,vendored,__init__.py,examples,benchmarks,napari/resources/_qt_resources*.py
jaimergp commented 2 years ago

@Czaki we have this issue https://github.com/napari/napari/issues/3602. Can you post it there too? It will be a perfect starting point. I think I will work on that next!

jaimergp commented 2 years ago

when you say "there", what are you referring to, https://github.com/conda-forge/napari-feedstock?

Yes, but in particular this WIP PR that I've been using to adapt the feedstock to our needs. The conda-forge bots won't pick up the changes in setup.cfg automatically, so just wanted to make sure we do not miss that. Also note there were some licensing issues I've also fixed in that PR we can port to the final release PR.

psobolewskiPhD commented 2 years ago

That said, it's a scary situation :D We haven't added any safeguards to the "base" env, so if you start modifying the installation you might end up with a non-functional napari. There are ways to make this happen, but I am not sure it's a path we want to pursue right now. It does have some synergies with the plugin isolation work.

But there is a env right? and the napari one? or is the napari one base.

jaimergp commented 2 years ago

But there is a env right? and the napari one? or is the napari one base.

napari is installed in the base env.

tlambert03 commented 2 years ago

The conda-forge bots won't pick up the changes in setup.cfg automatically, so just wanted to make sure we do not miss that.

yep, aware. we generally do a manually check on that each time

jaimergp commented 2 years ago

ARM64 builds failing because pytomlpp is not available for that platform yet. Off to the foooorge 🔨

tlambert03 commented 2 years ago

ARM64 builds failing because pytomlpp is not available for that platform yet. Off to the foooorge 🔨

darn... I specifically picked (over the other fast toml parses) pytomlpp over the alternatives because it had good support on conda forge. If you'd prefer, I can release a version of npe2 that falls back to toml if pytomlpp isn't available, then we can skip on arm?

jaimergp commented 2 years ago

Don't worry for now. It might be a simple build after all. It it gets complicated we can reevaluate! Thanks @tlambert03!

psobolewskiPhD commented 2 years ago

ARM64 builds failing because pytomlpp is not available for that platform yet. Off to the foooorge 🔨

Let me know if you need something tested!

sofroniewn commented 2 years ago

@sofroniewn -- could we sync a bit before the final release to make sure the conda-forge recipe is accurate?

Great progress here! Let me know if you still need more info. I'm assuming this is "release" of bundle, not of 0.4.13, which should hopefully get released in a few hours

jaimergp commented 2 years ago

I'm assuming this is "release" of bundle, not of 0.4.13, which should hopefully get released in a few hours

A bit like of both. I would like to include the license updates in the 0.4.13 release. Right now the vendored packages are not shipping the license metadata and that's problematic. See here, @sofroniewn

sofroniewn commented 2 years ago

I would like to include the license updates in the 0.4.13 release. Right now the vendored packages are not shipping the license metadata and that's problematic

hmm ok - is that something that will require a PR to this repo or can be "fixed" after the release. maybe we discuss in release stream on zulip, as i think otherwise we are ready to go

jaimergp commented 2 years ago

is that something that will require a PR to this repo or can be "fixed" after the release.

For the record is just a conda-forge change, nothing on this repo. But yea, let's do it with a proper RC cadence and not just before the final 😬 My bad!

sofroniewn commented 2 years ago

For the record is just a conda-forge change, nothing on this repo. But yea, let's do it with a proper RC cadence and not just before the final 😬 My bad!

All good, as said in zulip, i think 0.4.14 will probably come pretty soon, say next week. we often do a bug fix release fairly quick after a large one

jaimergp commented 2 years ago

https://github.com/conda-forge/pytomlpp-feedstock/pull/9# has been merged. ARM64 builds should be working again in an hour or so.

psobolewskiPhD commented 2 years ago

I'm spamming mamba repoquery search pytomlpp !! It's here:

Executing the query pytomlpp

conda-forge/osx-arm64    [====================] (00m:00s) Done
conda-forge/noarch       [====================] (00m:01s) Done

 Name     Version Build           Channel              
────────────────────────────────────────────────────────
 pytomlpp 1.0.10  py310hd99b56e_0 conda-forge/osx-arm64
 pytomlpp 1.0.10  py38h1670459_0  conda-forge/osx-arm64
 pytomlpp 1.0.10  py39h4d2d688_0  conda-forge/osx-arm64
jaimergp commented 2 years ago

It worked! Installers will be available here: https://github.com/napari/napari/actions/runs/1714793319

If everything goes ok with this round of updates, I think we can consider this PR ready for review before the week ends!

psobolewskiPhD commented 2 years ago

Flawless! Everything worked perfectly. No warnings, no authorizations needed. First run is slow, but it works perfectly!

image
Czaki commented 2 years ago

Bundle-specific code paths. Simple tests run fine without any sentinel files. Maybe not needed anymore outside of briefcase land? The installation is technically not frozen as PyInstaller does, it's a full conda environment, so it should behave as that kind of installation too.

briefacse is also not a frozen environment. so if some paths depend on sys.frozen paths are for pyinstaller and are most probably introduced based on my requests. I still use pyinstaller and it is possible that another person also does this as pyinstaller has one big advantage. the application could be unpacked and instantly used. Without the need to do an installation. So it is nice to provide a ready environment to an inexperienced user.

jni commented 2 years ago
  • What do we do with the Briefcase-specific code? Should we delete bundle.py and its CI file? Keep it around for a while? Both pipelines are independent now.

I'm agnostic about this, and I guess the question is how confident we are that this is the way forward. If it's "very", and I think it is, then I'd just remove things. It is always recoverable from git.

(And indeed I would rename bundle_conda.py to just bundle.py.)

  • Bundle-specific code paths. Simple tests run fine without any sentinel files. Maybe not needed anymore outside of briefcase land? The installation is technically not frozen as PyInstaller does, it's a full conda environment, so it should behave as that kind of installation too.

Which code paths are you referring to?

  • Release CI has not been tested with these bundles yet for obvious reasons. How do we approach this?

If we get users to test the bundle + some plugins on all platforms, I'm comfortable updating release CI to use these.

  • The CI takes a bit now (slower than briefcase due to the interim conda builds). Do we want to run it for every PR push?

Possibly not. Maybe build only if "bundle" tag is applied?

  • We use a feedstock PR to source the conda recipe for our own internal builds (nightlies, PRs). That recipe is heavily modified and contains the shortcut metadata. We need to decide where to store that metadata. In the recipe itself, or maybe in our repo, under resources?

I don't have an opinion here. Does it affect the conda-installed napari at all to have this data in the feedstock? But I think resources might be better because it's more about the bundle than about conda-installing napari?

  • What about the bundled licenses? Should we fix that issue in this PR or a separate one? Essentially, we need to provide a central place where all bundled licenses are enumerated. See napari/napari#3875.

Probably this PR, I was going to comment on that, so good call. =)

jaimergp commented 2 years ago

Regarding the Windows signing, this is what we are doing so far and why:

One weird thing I noticed is that the warning does not appear under some conditions!

Example output of a failing signtool verify run:

signtool.exe verify /v napari-0.4.14.dev76-Windows-x86_64.exe

Verifying: napari-0.4.14.dev76-Windows-x86_64.exe

Signature Index: 0 (Primary Signature)
Hash of file (sha256): 44F2E9EADCF52DDA6EEC78EFBD01AA9C6A065EEC72954066050AD72BF56DA25F

Signing Certificate Chain:
    Issued to: Apple Root CA
    Issued by: Apple Root CA
    Expires:   Fri Feb 09 22:40:36 2035
    SHA1 hash: 611E5B662C593A08FF58D14AE22452D198DF6C60

        Issued to: Developer ID Certification Authority
        Issued by: Apple Root CA
        Expires:   Mon Feb 01 23:12:15 2027
        SHA1 hash: 3B166C3B7DC4B751C9FE2AFAB9135641E388E186

            Issued to: Developer ID Application: Chan Zuckerberg Initiative, LLC (7WAT66VT96)
            Issued by: Developer ID Certification Authority
            Expires:   Thu Nov 05 15:35:08 2026
            SHA1 hash: AB6E2BF53AD3C81A29C60B71BE3C1E66BAEDC57C

The signature is timestamped: Mon Jan 24 11:25:25 2022
Timestamp Verified by:
    Issued to: USERTrust RSA Certification Authority
    Issued by: USERTrust RSA Certification Authority
    Expires:   Tue Jan 19 00:59:59 2038
    SHA1 hash: 2B8F1B57330DBBA2D07A6C51F70EE90DDAB9AD8E

        Issued to: Sectigo RSA Time Stamping CA
        Issued by: USERTrust RSA Certification Authority
        Expires:   Tue Jan 19 00:59:59 2038
        SHA1 hash: 02D65B95E28370C1570095FA88F923DD937FAD8F

            Issued to: Sectigo RSA Time Stamping Signer napari/napari#2
            Issued by: Sectigo RSA Time Stamping CA
            Expires:   Fri Jan 23 00:59:59 2032
            SHA1 hash: 951137101D882F31BD513F949ADA4C68AD8C08F5

SignTool Error: A certificate chain processed, but terminated in a root
        certificate which is not trusted by the trust provider.

Number of files successfully Verified: 0
Number of warnings: 0
Number of errors: 1
jaimergp commented 2 years ago

For the record, the error on Windows right now is unrelated to us:

Signing installer with D:\a\_temp/certificate.pfx
signtool (stdout):
Done Adding Additional Store
Error information: "Error: SignerSign() failed." (-2146869243/0x80096005)

signtool (stderr):
SignTool Error: An unexpected internal error has occurred.

This seems to happen when the timestamp server for the signature cannot be reached, so I am guessing that it's down temporarily. Will retry tomorrow.

jaimergp commented 2 years ago

Answering @jni:

I'm agnostic about this, and I guess the question is how confident we are that this is the way forward. If it's "very", and I think it is, then I'd just remove things. It is always recoverable from git.

I think we can safely delete, but for the sake of debugging easily in the first weeks, maybe we should have both around. Maybe not in the release itself, but as a CI artifact?

Which code paths are you referring to?

Parts that use this function. But @Czaki raised a good point that some other projects might be freezing / bundling napari in ways that require these workarounds. I am just not sure where the maintenance burden should rest on. We don't need this for our constructor based installers.

Maybe build only if "bundle" tag is applied?

Yep, I like this. Hopefully once we have tested them for a couple weeks?

I don't have an opinion here. Does it affect the conda-installed napari at all to have this data in the feedstock? But I think resources might be better because it's more about the bundle than about conda-installing napari?

The feedstock doesn't care about where the data comes from as long as it's there when we start building. Technically a conda user can choose to install napari-menu and once the ecosystem has caught up on the new menuinst, it will work.

Probably this PR, I was going to comment on that, so good call. =)

I have added a new CI step that cats all the licenses. Of course this is not final, but gives you an idea of the data we are dealing with. Where should I write this too? The docs? The website?

Thanks!

jaimergp commented 2 years ago

I noticed this PR has some pngs in it - can we move them over to git-lfs, or does that now happen automatically for us?

I am not too familiar with LFS but the config in .gitattributes looks automatic to me. Do I need to rebase or something like that?

jaimergp commented 2 years ago

Amusingly, shortcuts worked out of the box on MacOS. I still need to check Windows and Linux, but I don't expect surprises. This means multi-env installs are now a reality. Coming next: the launcher thingy.

That said, I ran into an issue with main and put napari/napari#4144 to fix it. So the artifacts you can find in the last CI run won't work if you try them now.

jni commented 2 years ago

I noticed this PR has some pngs in it - can we move them over to git-lfs, or does that now happen automatically for us?

I am not too familiar with LFS but the config in .gitattributes looks automatic to me. Do I need to rebase or something like that?

You would need to rebase or merge main in and have git-lfs installed for the config settings to take effect, but, resources/ is specifically excluded from lfs since it is needed for the app rather than the docs, so these pngs won't go into lfs. They're small and I'm not too worried about them, but I do wonder @jaimergp whether they could just be dynamically generated at bundle build time from the logo that is already there?

jni commented 2 years ago

Oh, weird, no, resources is not excluded, see this PR. What is excluded is napari/resources/ and that is presumably where these pngs belong... But again, probably they should just be autogenerated.

jaimergp commented 2 years ago

Sure, we can generate them on the spot. I'll work on it.

jaimergp commented 2 years ago

Image auto-generation should be working now! I'll double check when napari/napari#4144 is merged.

jaimergp commented 2 years ago

Multi-env installers confirmed to be working across all operating systems. I also fixed a Windows uninstaller issue with the shortcuts, and a little bug on Linux.

goanpeca commented 2 years ago

@jaimergp is the PR ready for another final round of reviews or are we missing something?

Thanks!

jaimergp commented 2 years ago

Pending tasks:

liu-ziyang commented 2 years ago

@jaimergp can you please take a look at the pre-commit test, so we can freeze the code and rally final round of review? thanks!

jaimergp commented 2 years ago

Done!

jni commented 2 years ago

Well, that was pretty massive! :sweat_smile: :rocket: :moon:

goanpeca commented 2 years ago

@jni could you please hold the new release for tomorrow there is a pending PR?

Thanks!