Define separate entrypoints for translator plugins and make requirements optional

[dhomeier]

Description

Giving each plugin its own entry point so it can be loaded on demand/availability.
This allows making regions, specutils, specreduce and spectral-cube optional dependencies; they will also be installed with the glue-astronomy[all] and glue-astronomy[test] options.

To be discussed if there is a preferable options.extras_require setup or if the tests should also just try the imports and pass, if not installed.

[codecov]

Codecov Report

Merging #76 (b9e1785) into main (d245375) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   97.46%   97.47%   +0.01%     
==========================================
  Files          17       17              
  Lines        1223     1230       +7     
==========================================
+ Hits         1192     1199       +7     
  Misses         31       31              

Impacted Files	Coverage Δ
glue_astronomy/__init__.py	100.00% <ø> (ø)
glue_astronomy/conftest.py	100.00% <100.00%> (ø)
glue_astronomy/translators/__init__.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dhomeier]

@pllim if you want to check this for your dependency tree...

[pllim]

I don't understand this. If they are only used in tests now, why pollute __init__.py and not just make them test fixtures?

[astrofrog]

They are not just used by tests, these functions are executed by the glue plugin loader

[pllim]

Ah, okay... I see that these invoke some entry point dark magic. Nvm.

[pllim]

I checked out this branch and then do a clean install using pip install -e . of this branch but it still pulls in spectral-cube .

Installing collected packages: spectral-cube, glue-astronomy
  Running setup.py develop for glue-astronomy
Successfully installed glue-astronomy-0.4.1.dev15+gb9e1785 spectral-cube-0.6.0

[pllim]

I think this is good but doesn't do anything because glue-core else stills pulls in spectral-cube , as Derek suspected. See glue-viz/glue#2316

[dhomeier]

Yes, apparently when running without --upgrade option in the conda environment pip was still happy with the old conda-installed 1.0.1. Once I pull in a pip-installed glue-core (updating to 1.5.0), it inevitably installs spectral-cube as well.

[pllim]

Also, I think this breaks translator lookup on Jdaviz. I forgot to uninstall this branch and was working on something else and glue complained that it cannot find translator for Spectrum1D .

[dhomeier]

Is specutils installed in that branch?

[pllim]

Yes, specutils 1.7.1.dev (though might not be the latest dev).

Once I uninstalled this branch and installed the released glue-astronomy , things worked again.

[dhomeier]

Can you pin that down to a failure within glue, or does it only happen in the jdaviz environment?

FWIW glue tests are passing locally for me with no glue-astronomy installed at all, with this branch I am getting e.g. in test_data_translation.py

=================================================================== short test summary info ====================================================================
FAILED glue/core/tests/test_data_translation.py::TestTranslationData::test_get_object_explicit_class - AssertionError: assert 'Specify the ...akeDataObject' ...
FAILED glue/core/tests/test_data_translation.py::TestTranslationData::test_get_subset_object_explicit_class - AssertionError: assert 'Specify the ...akeDataO..
================================================== 2 failed, 10 passed, 2007 deselected, 7 warnings in 0.84s ===================================================

but with released glue-astronomy versions (0.4 or 0.1), it's even one more failure!

FAILED glue/core/tests/test_data_translation.py::TestTranslationData::test_get_object_explicit_class - AssertionError: assert 'Specify the ...akeDataObject' ...
FAILED glue/core/tests/test_data_translation.py::TestTranslationData::test_get_subset_object_explicit_class - AssertionError: assert 'Specify the ...akeDataO...
FAILED glue/core/tests/test_data_translation.py::TestTranslationData::test_get_selection_invalid - assert "Subset state...t_translator'" == "Subset state...t...
=================================================== 3 failed, 9 passed, 2007 deselected, 7 warnings in 0.93s ===================================================

Apparently glue-core has no test setup with glue-astronomy installed defined.

[pllim]

Since you can get error in glue-core, do you still need me to dig at Jdaviz? If so, please lemme know and I'll try tomorrow. Thanks!

glue was complaining it can't find translator for Spectrum1D but I don't have the traceback right now.

[dhomeier]

Yes, would be good to know what did work in 0.4.0 and broke now – I only find 3 identical failures in all of glue.core, and this one with 0.4.0 that no longer fails with 0.4.1dev. Thanks!

________________________________________________________ TestTranslationData.test_get_selection_invalid ________________________________________________________
...
        with pytest.raises(ValueError) as exc:
            data.get_selection_definition(subset_id=0)
>       assert exc.value.args[0] == ("Subset state handler format not set - should be one "
                                     "of:\n\n* 'my_subset_translator'")
E       assert ('Subset state handler format not set - should be one of:\n'\n '\n'\n "* 'astropy-regions'\n"\n "* 'my_subset_translator'") == ('Subset state handler format not set - should be one of:\n'\n '\n'\n "* 'my_subset_translator'")
E           Subset state handler format not set - should be one of:
E           
E         + * 'astropy-regions'
E           * 'my_subset_translator'

[pllim]

Okay, here is the traceback with this branch:

from astropy.utils.data import download_file
from jdaviz import Cubeviz

fn = download_file('https://stsci.box.com/shared/static/28a88k1qfipo4yxc4p4d40v4axtlal8y.fits', cache=True)

cubeviz = Cubeviz()
cubeviz.load_data(fn)

----> 5     cubeviz.load_data(fn)

.../jdaviz/configs/cubeviz/helper.py in load_data(self, data, **kwargs)
     50             raise RuntimeError('only one cube may be loaded per Cubeviz instance')
     51 
---> 52         super().load_data(data, parser_reference="cubeviz-data-parser", **kwargs)
     53 
     54     def select_slice(self, slice):

.../jdaviz/core/helpers.py in load_data(self, data, parser_reference, **kwargs)
     71 
     72     def load_data(self, data, parser_reference=None, **kwargs):
---> 73         self.app.load_data(data, parser_reference=parser_reference, **kwargs)
     74 
     75     @property

.../jdaviz/app.py in load_data(self, file_obj, parser_reference, **kwargs)
    516                 # If the parser returns something other than known, assume it's
    517                 #  a message we want to make the user aware of.
--> 518                 msg = parser(self, file_obj, **kwargs)
    519 
    520                 if msg is not None:

.../jdaviz/configs/cubeviz/plugins/parsers.py in parse_data(app, file_obj, data_type, data_label)
     75 
     76             else:
---> 77                 _parse_hdulist(app, hdulist, file_name=data_label or file_name)
     78 
     79     # If the data types are custom data objects, use explicit parsers. Note

.../jdaviz/configs/cubeviz/plugins/parsers.py in _parse_hdulist(app, hdulist, file_name)
    177 
    178         sc = _return_spectrum_with_correct_units(flux, wcs, metadata, data_type, hdulist=hdulist)
--> 179         app.add_data(sc, data_label)
    180         if data_type == 'flux':  # Forced wave unit conversion made it lose stuff, so re-add
    181             app.data_collection[-1].get_component("flux").units = flux_unit

.../jdaviz/app.py in add_data(self, data, data_label, notify_done)
    893             data_label = data.label
    894         data_label = data_label or "New Data"
--> 895         self.data_collection[data_label] = data
    896 
    897         # Send out a toast message

.../glue/core/data_collection.py in __setitem__(self, key, data)
    427         if not isinstance(data, Data):
    428 
--> 429             handler, preferred = data_translator.get_handler_for(data)
    430 
    431             data = handler.to_data(data)

.../glue/config.py in get_handler_for(self, data_or_class)
    565                                 "type Data to {0}".format(data_or_class.__name__))
    566             else:
--> 567                 raise TypeError("Could not find a class to translate objects of "
    568                                 "type {0} to Data".format(data_or_class.__class__.__name__))
    569         return handler, preferred

TypeError: Could not find a class to translate objects of type Spectrum1D to Data

[dhomeier]

--> 895 self.data_collection[data_label] = data

OK. I note that data_collection['spectrum'] = spectrum1d by itself does not work with current glue+glue_astronomy;
jdaviz/core/__init__.py is doing an import glue_astronomy.translators to register those translators, which previously did import 'ccddata', 'regions', 'spectral_cube', 'spectrum1d', 'trace'
but with this PR instead there are only
'setup_ccddata', 'setup_regions', 'setup_spectral_cube', 'setup_spectrum1d', 'setup_trace'
Could call all the methods in translators.__init__.py to do the actual import, similar to what is done in conftest.py, but to keep the dependencies optional, that would require a

try:
    setup_spectrum1d()
except ImportError:
    pass

and so on for every single translator (would there even be a point in having separate entry points then?)!

@astrofrog is the above import as done by jdaviz.core.__init__() the recommended way to register available translators, or is there an alternative within glue-core somehow?

[astrofrog]

@dhomeier - jdaviz should not be importing glue_astronomy.translators but instead jdaviz should really just be calling load_plugins from glue-core. If there is a desire to specify a limited list of plugins to load, we should modify load_plugins to take an optional list of plugins to load.

[pllim]

jdaviz should really just be calling load_plugins from glue-core

Where is the documentation telling us how to do this? I cannot find it.

[dhomeier]

jdaviz should really just be calling load_plugins from glue-core

Where is the documentation telling us how to do this? I cannot find it.

I think the idea is to first import glue_astronomy and then call load_plugins(), which seems to work with 0.5.0, but not with this branch – so something here still needs to be fixed.
@astrofrog is it because the functions need to be called setup after all? I tried putting spectrum1d into its own subdirectory with its setup() function (so the plugin becomes spectrum1d = glue_astronomy.translators.spectrum1d:setup), but this is still not called automatically on import glue_astronomy.
On current main in contrast there is the glue_astronomy = glue_astronomy:setup plugin, which then does the from glue_astronomy import translators – how would that be substituted in this scheme?

[astrofrog]

@astrofrog is it because the functions need to be called setup after all? I tried putting spectrum1d into its own subdirectory with its setup() function (so the plugin becomes spectrum1d = glue_astronomy.translators.spectrum1d:setup), but this is still not called automatically on import glue_astronomy.

Entry points are not executed on glue_astronomy import but instead when load_plugins is called in glue-core. If glue-astronomy is installed, it does not need to be imported explicitly and load_plugins is sufficient.

As mentioned above, it would probably be nice to have a way of specifying a list or a single plugin to load in load_plugins, and perhaps also a way to easily list plugins with a list_plugins function.

[astrofrog]

We should get this moving again - I've left a few comments below. To keep things simpler discussion wise, I would suggest removing the change related to making the dependencies optional and focusing for now on making the plugins individually defined.

In glue-viz/glue#2447 I've mentioned that we need to improve load_plugins for use downstream in e.g. jdaviz.

[astrofrog]

We should give slightly more explicit names to the plugins so that things are clear when we are able to load them individually in future. Translators should probably be called e.g. ccddata_translator

[astrofrog]

I think spectral_cube_io is fine as it covers the data factory and the command-line parser

[astrofrog]

trace is not very clear (not your fault but we should have named it better in glue-astronomy in the first place). I think we should probably rename all instances of trace (or related classes) to something like specreduce_trace - then the translator should be specreduce_trace_translator.