Define separate entrypoints for translator plugins and make requirements optional #76

dhomeier · 2022-08-17T18:23:50Z

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 · 2022-08-17T18:28:27Z

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 · 2022-08-17T19:13:00Z

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

pllim · 2022-08-17T19:29:41Z

glue_astronomy/translators/__init__.py

-from . import spectral_cube  # noqa
-from . import spectrum1d  # noqa
-from . import trace # noqa
+def setup_ccddata():


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

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

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

pllim · 2022-08-17T20:33:50Z

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 · 2022-08-17T20:53:25Z

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 · 2022-08-17T22:02:16Z

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 · 2022-08-17T22:04:34Z

Is specutils installed in that branch?

pllim · 2022-08-17T22:06:35Z

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 · 2022-08-17T22:52:10Z

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 · 2022-08-17T22:54:09Z

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 · 2022-08-17T23:22:33Z

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 · 2022-08-18T01:17:34Z

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 · 2022-08-18T12:38:42Z

--> 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 · 2022-08-23T14:22:56Z

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 · 2022-08-24T12:14:14Z

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 · 2023-06-15T14:15:31Z

@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 · 2023-10-18T10:16:47Z

setup.cfg

 qt =
    PyQt5

 [options.entry_points]
 glue.plugins =
-    glue_astronomy = glue_astronomy:setup
-    spectral_cube = glue_astronomy.io.spectral_cube:setup
+    ccddata = glue_astronomy.translators:setup_ccddata


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 · 2023-10-18T10:17:49Z

setup.cfg

+    regions = glue_astronomy.translators:setup_regions
+    spectrum1d = glue_astronomy.translators:setup_spectrum1d
+    spectral_cube = glue_astronomy.translators:setup_spectral_cube
+    spectral_cube_io = glue_astronomy.io.spectral_cube:setup


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

astrofrog · 2023-10-18T10:19:13Z

setup.cfg

+    spectrum1d = glue_astronomy.translators:setup_spectrum1d
+    spectral_cube = glue_astronomy.translators:setup_spectral_cube
+    spectral_cube_io = glue_astronomy.io.spectral_cube:setup
+    trace = glue_astronomy.translators:setup_trace


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.

dhomeier added the enhancement New feature or request label Aug 17, 2022

dhomeier requested a review from astrofrog August 17, 2022 18:23

dhomeier force-pushed the plugin-entrypoints branch from a610506 to de60544 Compare August 17, 2022 18:35

Define separate entrypoints for plugins

b9e1785

dhomeier force-pushed the plugin-entrypoints branch from de60544 to b9e1785 Compare August 17, 2022 18:47

pllim reviewed Aug 17, 2022

View reviewed changes

pllim mentioned this pull request Aug 17, 2022

pvextractor should not be in install_requires glue-viz/glue#2316

Closed

pllim suggested changes Aug 17, 2022

View reviewed changes

dhomeier marked this pull request as draft August 18, 2022 19:15

astrofrog reviewed Aug 23, 2022

View reviewed changes

astrofrog mentioned this pull request Oct 18, 2023

Document and improve load_plugins glue-viz/glue#2447

Open

astrofrog requested changes Oct 18, 2023

View reviewed changes

pllim mentioned this pull request Aug 13, 2024

[BUG]I think Cubeviz might be incompatible with the latest version of astropy spacetelescope/jdaviz#3150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define separate entrypoints for translator plugins and make requirements optional #76

Define separate entrypoints for translator plugins and make requirements optional #76

dhomeier commented Aug 17, 2022

codecov bot commented Aug 17, 2022 •

edited

Loading

dhomeier commented Aug 17, 2022

pllim Aug 17, 2022

astrofrog Aug 17, 2022

pllim Aug 17, 2022

pllim commented Aug 17, 2022

pllim left a comment

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022 •

edited

Loading

dhomeier commented Aug 17, 2022 •

edited

Loading

pllim commented Aug 18, 2022

dhomeier commented Aug 18, 2022 •

edited

Loading

astrofrog left a comment

pllim commented Aug 23, 2022

dhomeier commented Aug 24, 2022

astrofrog commented Jun 15, 2023

astrofrog left a comment

astrofrog Oct 18, 2023

astrofrog Oct 18, 2023

astrofrog Oct 18, 2023

Define separate entrypoints for translator plugins and make requirements optional #76

Are you sure you want to change the base?

Define separate entrypoints for translator plugins and make requirements optional #76

Conversation

dhomeier commented Aug 17, 2022

Description

codecov bot commented Aug 17, 2022 • edited Loading

Codecov Report

dhomeier commented Aug 17, 2022

pllim Aug 17, 2022

Choose a reason for hiding this comment

astrofrog Aug 17, 2022

Choose a reason for hiding this comment

pllim Aug 17, 2022

Choose a reason for hiding this comment

pllim commented Aug 17, 2022

pllim left a comment

Choose a reason for hiding this comment

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022

dhomeier commented Aug 17, 2022

pllim commented Aug 17, 2022 • edited Loading

dhomeier commented Aug 17, 2022 • edited Loading

pllim commented Aug 18, 2022

dhomeier commented Aug 18, 2022 • edited Loading

astrofrog left a comment

Choose a reason for hiding this comment

pllim commented Aug 23, 2022

dhomeier commented Aug 24, 2022

astrofrog commented Jun 15, 2023

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog Oct 18, 2023

Choose a reason for hiding this comment

astrofrog Oct 18, 2023

Choose a reason for hiding this comment

astrofrog Oct 18, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2022 •

edited

Loading

pllim commented Aug 17, 2022 •

edited

Loading

dhomeier commented Aug 17, 2022 •

edited

Loading

dhomeier commented Aug 18, 2022 •

edited

Loading