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

admin/update-min-aicsimageio-ver-and-move-to-gpl3 #36

Merged
merged 5 commits into from
Feb 19, 2022

Conversation

evamaxfield
Copy link
Collaborator

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • Link to any relevant issue in the PR description. Ex: Resolves [admin/build-py39 #12], adds tiff file format support

Resolves #35

  • Provide context of changes.

Sets the new license to GPL because we are upgrading to aicsimageio v4.4.0 and installing all optional reader deps.
Additionally fixes and updates some general README info.

  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.

Tagging @psobolewskiPhD @tlambert03. Any thoughts?

Thanks for contributing!

@evamaxfield evamaxfield added the documentation Improvements or additions to documentation label Oct 12, 2021
@evamaxfield evamaxfield requested a review from toloudis October 12, 2021 15:47
@evamaxfield evamaxfield self-assigned this Oct 12, 2021
@evamaxfield
Copy link
Collaborator Author

Ignoring the failing builds. Will rerun when 4.4.0 is actually pushed to pypi

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

Merging #36 (6f04345) into main (7605974) will decrease coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   83.15%   83.05%   -0.11%     
==========================================
  Files           7        5       -2     
  Lines         184      177       -7     
==========================================
- Hits          153      147       -6     
+ Misses         31       30       -1     
Impacted Files Coverage Δ
napari_aicsimageio/tests/__init__.py
napari_aicsimageio/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7605974...6f04345. Read the comment docs.

@psobolewskiPhD
Copy link
Collaborator

First, I'm incredibly flattered you listed me as a contributor in AICS 4.3 when I made a just a tiny tweak to a thing that in the same release became not a thing. 🤣

I can't say I'm fully able to wrap my head around the GPL vs. BSD etc, issues.
That said, I do think that making all the readers available via the napari GUI would be a huge boon to users, including people I work with (I do understand bio-formats still requires a Java install, which can be a sticking point, but that's sort of a fallback anyways.)
So I guess I come back to @tlambert03 comment about making this plugin a dependency for napari at some point. If GPL license—which I understand sort of propagates?—would make that impossible, then long-term this could be a negative. But if that's a long way off...

@tlambert03
Copy link
Contributor

yeah, I'm fine with this. If it later becomes a sticking point, we can create a BSD version at that time

@evamaxfield
Copy link
Collaborator Author

Holding for a week. Allen Legal looking into the ramifications of converting to a GPL license. (I.e. "how easy is it to move back to BSD after switching to GPL)

@evamaxfield evamaxfield mentioned this pull request Feb 19, 2022
5 tasks
@evamaxfield evamaxfield removed the request for review from toloudis February 19, 2022 16:19
@psobolewskiPhD
Copy link
Collaborator

psobolewskiPhD commented Feb 19, 2022

OK, so this is sort of insane, but it works...
What if, the error from napari-aicsimageio when LIF or CZI is tried, but the extras are not installed (in other words, the default install, without the GPL parts) told the use to open the console and do:
%pip install readlif
or
%pip install aicspylibczi
(this is from: ipython/ipython#11524)
It works! You don't even need to relaunch napari! You can see console output and the scene selector of a LIF.
image

Edit: here's pip show from the terminal, showing that it installed in the right env.

─ ~/dev ······················································· ✔  3m 44s ─╮
╰─ pip show readlif                                               (napari-CL) ─╯
Name: readlif
Version: 0.6.5
Summary: Fast Leica LIF file reader written in python
Home-page: /~https://github.com/nimne/readlif
Author: Nick Negretti
Author-email: nick.negretti@gmail.com
License: GPLv3
Location: /Users/piotrsobolewski/Dev/miniforge3/envs/napari-CL/lib/python3.9/site-packages
Requires: Pillow
Required-by: 

@evamaxfield
Copy link
Collaborator Author

OK, so this is sort of insane, but it works... What if, the error from napari-aicsimageio when LIF or CZI is tried, but the extras are not installed (in other words, the default install, without the GPL parts) told the use to open the console and do: %pip install readlif or %pip install aicspylibczi (this is from: ipython/ipython#11524) It works! You don't even need to relaunch napari! You can see console output and the scene selector of a LIF. image

Edit: here's pip show from the terminal, showing that it installed in the right env.

─ ~/dev ······················································· ✔  3m 44s ─╮
╰─ pip show readlif                                               (napari-CL) ─╯
Name: readlif
Version: 0.6.5
Summary: Fast Leica LIF file reader written in python
Home-page: /~https://github.com/nimne/readlif
Author: Nick Negretti
Author-email: nick.negretti@gmail.com
License: GPLv3
Location: /Users/piotrsobolewski/Dev/miniforge3/envs/napari-CL/lib/python3.9/site-packages
Requires: Pillow
Required-by: 

Uhhhhhh can you make a PR for that?

@evamaxfield
Copy link
Collaborator Author

Errr wait, this PR is adding all the installs. No reason to add that once this is merged and deployed

@evamaxfield
Copy link
Collaborator Author

I have no idea why the tests are failing on Windows... but I am going to ship this and release so that it unblocks a lot of people.

Will make an issue for fixing windows tests.

@evamaxfield evamaxfield merged commit ade12fa into main Feb 19, 2022
@evamaxfield evamaxfield deleted the admin/upgrade-min-aicsimageio branch February 19, 2022 16:54
@evamaxfield
Copy link
Collaborator Author

@psobolewskiPhD release is out v0.5.0.

If you have a chance, can you make a fresh env and let me know if everything installs and runs correctly.

I am technically out of town this week so will be sporadic in response but hopefully everything works for you!

@psobolewskiPhD
Copy link
Collaborator

Everything is golden!
Works perfectly, both readlif and aicspylibczi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: license changes
4 participants