XENONnT / straxen

Streaming analysis for XENON
BSD 3-Clause "New" or "Revised" License
19 stars 33 forks source link

Move holoviews/jupyter dependencies to a seperate package? #475

Open JoranAngevaare opened 3 years ago

JoranAngevaare commented 3 years ago

It might be worth disentangling these dependencies from straxen, arguably even matplotlib to thereby keep the code clearly focused on the data-access and processing.

These two things currently in straxen are arguably very different in nature and may require other standards e.g. for testing etc.

WenzDaniel commented 3 years ago

Fully agree on that point, but as you mentioned I would even move matplotlib to another package. I already looked a bit into different solutions for this and I would like to try to use package extras, like e.g. hypothesis[numpy]. I found a thread about this here, but I am just at the beginning and need a bit more time.

jmosbacher commented 3 years ago

Just wanted to comment that I think this is a great idea. If you want to make the transition as transparent as possible to analysts I can think of two options: 1) Only removing the dependencies like @WenzDaniel mentioned, there is a mechanism for this in setuptools. This means people would opt-in to the extra packages. The downside to this is that the plotting code stays in straxen and therefore you have additional complexity of the code that needs to check whether dependencies were install and raise a clear enough error when you try to call a function that needs a dependency not installed. If the errors are not very clear you will cause a lot of pain to analysts not understanding why some functions arent working. 2) Use the plugin system (the python one, not the strax one) using entrypoints you can have a plotting package add the plotting functions to straxen when it is installed. That would completely separate the code and user that didnt install the plotting package wont see the extra function at all.

I have used both of these methods in the past so would be happy to help with implementation, both are pretty easy to do .

JoranAngevaare commented 3 years ago

Hi @jmosbacher , yeah it would be great if we can learn from your experience. The biggest concern I would have is if we could maintain the context-minianalysis wrapper as we currently have.

Ideally, we still wrap that code to the context. From the two options you list, would you say either makes for a better candidate to achieve this?

jmosbacher commented 3 years ago

@jorana i think in both cases you can keep the minianalysis wrapper the way it is. e.g. if you want to use the plugin framework you just add some code like this:

for entry_point in pkg_resources.iter_entry_points('straxen.minialalysis'):
    requires, hv_bokeh, warn_beyond_sec,default_time_selection, f = entry_point.load()
    _ = straxen.mini_analysis(requires, hv_bokeh, warn_beyond_sec,default_time_selection)(f)

That loads minianalyses from any plugin packages that are installed. The plugin packages themselves register the minianalyses in their setup.py entrypoints as followes:

entry_points={ "straxen.minialalysis": ["WaveformDisplay = xeplots.waveforms:WaveformDisplay"]},

Where the left hand side is the namespace (thats how straxen finds the pakcages that have minianalyses, notice its the same namespace in the straxen side code example) and the right hand side points to the path inside the package that contains the "plugin" that needs to be loaded.

The name on the left of the equal sign is accessible via entry_point.name

Of course the actual implementation can be a bit smarter than this, we can instead adapt the wrapper to accept also classed and take the parameters from the class instead of the plugin writer having to return a tuple of all the options.