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

NPE1Adapter Part 3 - caching of adapter manifests #126

Merged
merged 26 commits into from
Mar 23, 2022

Conversation

tlambert03
Copy link
Collaborator

@tlambert03 tlambert03 commented Mar 13, 2022

Breaking up #86 into three steps. This is part 3 (but it contains #124 and #125 which can be reviewed first)

This PR adds local caching of adapter manifests after contributions have been imported and indexed. This is the thing that will really make napari much faster on load time.

  • This lazily loaded manifest is used for the duration of the session, and also cached for use in the next session
  • caching will not be done for packages that are installed in editable mode.
  • the npe2 cache command provides additional ways to query/modify the cache
  • cache can be cleared by using npe2 cache --clear.
  • Cache can be ignored per session by starting napari with NPE2_NOCACHE=1

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #126 (0ae93cc) into main (39f00b9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #126   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         1650      1719   +69     
=========================================
+ Hits          1650      1719   +69     
Impacted Files Coverage Δ
npe2/_command_registry.py 100.00% <100.00%> (ø)
npe2/cli.py 100.00% <100.00%> (ø)
npe2/manifest/_npe1_adapter.py 100.00% <100.00%> (ø)

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 39f00b9...0ae93cc. Read the comment docs.

@tlambert03 tlambert03 requested a review from nclack March 14, 2022 13:32
@nclack
Copy link
Collaborator

nclack commented Mar 21, 2022

Testing this out I ran into something I don't understand. This is on a py39 env with napari main, this pr and napari_animation (which is npe1) installed. If I run python and import napari_animation I get:

% python
>>> import napari_animation
/Users/nclack/src/CZI/npe2/npe2/manifest/_npe1_shim.py:80: UserWarning: Failed to detect contributions for npe1 plugin 'napari-tools-menu': cannot import name 'get_settings' from partially initialized module 'napari.settings' (most likely due to a circular import) (/Users/nclack/src/CZI/napari/napari/settings/__init__.py)
  warnings.warn(

which seems strange? I'm out of time tonight, but I'll try to reproduce in the morning with a clean env.

On the success side, I can see e.g. napari-ndtiffs get cached. I really like that there's cli support.

Copy link
Collaborator

@nclack nclack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into an issue and got confused. Will pick this up in the am.

npe2/manifest/_npe1_shim.py Outdated Show resolved Hide resolved
npe2/manifest/_npe1_shim.py Outdated Show resolved Hide resolved
@tlambert03
Copy link
Collaborator Author

I'll take a look soon. I'll remind you that napari tools menu is not really a plugin, but rather a monkey patch that doesn't have hook implementations, so all bets are off for that one .

@nclack
Copy link
Collaborator

nclack commented Mar 21, 2022

reproduce in the morning with a clean env.

Did not reproduce. Works fine on a clean env.

Screen Shot 2022-03-21 at 9 10 47 AM

Copy link
Collaborator

@nclack nclack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. There was one spot where you hardcode the length of SHIM_NAME_PREFIX and I left a couple of questions.

One question I had was how to document some of the inferences that get made when converting plugins (like categorizing a theme as dark/light mode based on luma). Or extracting glob patterns for readers. I feel like that's a future discussion though.

npe2/manifest/_npe1_shim.py Outdated Show resolved Hide resolved
"""

assert shim_name.startswith(SHIM_NAME_PREFIX), f"Invalid shim name: {shim_name}"
python_name, idx = shim_name[13:].rsplit("_", maxsplit=1) # TODO, make a function
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what the TODO is referring to. I'm not sure I would factor anything out of this function as is. Should probably avoid the 13 - replace with len(SHIM_NAME_PREFIX) etc.

@tlambert03 tlambert03 changed the title NPE1Shim Part 3 - caching of shim manifests NPE1Adapter Part 3 - caching of adapter manifests Mar 23, 2022
@tlambert03
Copy link
Collaborator Author

/Users/nclack/src/CZI/npe2/npe2/manifest/_npe1_shim.py:80: UserWarning: Failed to detect contributions for npe1 plugin 'napari-tools-menu': cannot import name 'get_settings' from partially initialized module 'napari.settings' (most likely due to a circular import)

wasn't able to repeat in a clean environment with napari, npe2, and napari-animation... but if I pip install napari-tools-menu i do get it. will dig

@tlambert03
Copy link
Collaborator Author

wasn't able to repeat in a clean environment with napari, npe2, and napari-animation... but if I pip install napari-tools-menu i do get it. will dig

yeah, this circular import is because napari-tools-menu imports the private napari._qt module here. sorry, that's a "will not fix" 😂

@tlambert03
Copy link
Collaborator Author

merging!

This by no means implies that we can't iterate / refactor / cleanup moving forward. but it will make it a bit easier to get this in and play with it a bit

@tlambert03 tlambert03 merged commit 60bef97 into napari:main Mar 23, 2022
@tlambert03 tlambert03 deleted the shim3-cache branch March 23, 2022 22:37
@tlambert03 tlambert03 added the enhancement New feature or request label Apr 5, 2022
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants