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[bot]]

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 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

[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?

[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.