childmindresearch / niftyone

Large-scale neuroimaging visualization using FiftyOne
MIT License
4 stars 2 forks source link

Add plugin discovery #22

Open clane9 opened 2 months ago

clane9 commented 2 months ago

We need some way to discover user defined figures outside of the main niftyone namespace. This adds a very basic plugin discovery based on the naming convention niftyone_{plugin_name}. See the python packaging guide.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.72%. Comparing base (b3f7cb3) to head (95c0715).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #22 +/- ## ======================================= Coverage 99.72% 99.72% ======================================= Files 18 19 +1 Lines 726 730 +4 ======================================= + Hits 724 728 +4 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kaitj commented 1 month ago

Getting a chance to take a closer look at this now - this looks good to me. If I am understanding how it works, the plugin would be installed in the environment / part of the PYTHONPATH, prefixed with niftyone_ so that importlib can find it. More of a note for now, but we should make note of the naming convention in the documentation.

Theoretically, the plugin can be anything, but taking the DWI figure generation as an example - if it were to be a plugin, it'd be a package named niftyone_dwi, where actual creation of the figure could be part of the "ViewGenerator" class. I think as part of the intialization, we could check if it is an instance of the "ViewGenerator" class and call the register decorator on it if it is. The current generators (https://github.com/childmindresearch/niftyone/blob/9a301ef73a7e4684035bb3b0f8642aa8441cd740/src/niftyone/__init__.py#L8-L13) all do this from the function call themselves, so we could do this after the existing generators are registered or are we wanting the plugins to call the register decarator themselves (similar to the existing ones?)

clane9 commented 1 month ago

My thought was that the plugin code would mirror the internal code, so that it would be the plugin code's responsibility to inherit from the relevant base classes and then register. The benefit is that we don't need to do anything other than just import the plugin modules.

Ofc we will want to document this, but I think the documentation can wait until things are a bit more crystallized.

kaitj commented 1 month ago

Ahh that does make sense. I think I changed this in the PR that merges into this one, but that is an easy enough change to switch it back.