Add sunpy Longitude180 to allowed mixin classes #6619

Cadair · 2017-09-27T15:55:15Z

This enables ecsv roundtripping for SunPy coordinates.

This patches the following "bug":

from astropy.table import Table
from astropy.coordinates import SkyCoord
import astropy.units as u
import sunpy.coordinates

c = SkyCoord(lon=[-80, 80]*u.deg, lat=[-15,15]*u.deg, frame="heliographic_stonyhurst")

t = Table(data=[c])
t.write("test.ecsv", format="ascii.ecsv")
Table.read("test.ecsv", format="ascii.ecsv")

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-b7c054701dec> in <module>()
      8 t = Table(data=[c])
      9 t.write("test.ecsv", format="ascii.ecsv")
---> 10 Table.read("test.ecsv", format="ascii.ecsv")

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/table/table.py in read(cls, *args, **kwargs)
   2519         passed through to the underlying data reader (e.g. `~astropy.io.ascii.read`).
   2520         """
-> 2521         out = io_registry.read(cls, *args, **kwargs)
   2522         # For some readers (e.g., ascii.ecsv), the returned `out` class is not
   2523         # guaranteed to be the same as the desired output `cls`.  If so,

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/io/registry.py in read(cls, *args, **kwargs)
    529 
    530         reader = get_reader(format, cls)
--> 531         data = reader(*args, **kwargs)
    532 
    533         if not isinstance(data, cls):

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/io/ascii/connect.py in io_read(format, filename, **kwargs)
     37     from .ui import read
     38     format = re.sub(r'^ascii\.', '', format)
---> 39     return read(filename, format=format, **kwargs)
     40 
     41 

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/io/ascii/ui.py in read(table, guess, **kwargs)
    351                                              ' with fast (no guessing)'})
    352         else:
--> 353             dat = reader.read(table)
    354             _read_trace.append({'kwargs': new_kwargs,
    355                                 'status': 'Success with specified Reader class '

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/io/ascii/core.py in read(self, table)
   1200         if hasattr(self.header, 'table_meta'):
   1201             self.meta['table'].update(self.header.table_meta)
-> 1202         table = self.outputter(cols, self.meta)
   1203         self.cols = self.header.cols
   1204 

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/io/ascii/ecsv.py in __call__(self, cols, meta)
    190         # If no __mixin_columns__ exists then this function just passes back
    191         # the input table.
--> 192         out = serialize._construct_mixins_from_columns(out)
    193 
    194         return out

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/table/serialize.py in _construct_mixins_from_columns(tbl)
    198 
    199     for new_name, obj_attrs in mixin_cols.items():
--> 200         _construct_mixin_from_columns(new_name, obj_attrs, out)
    201 
    202     # If no quantity subclasses are in the output then output as Table.

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/table/serialize.py in _construct_mixin_from_columns(new_name, obj_attrs, out)
    152                 data_attrs_map[val['name']] = name
    153             else:
--> 154                 _construct_mixin_from_columns(name, val, out)
    155                 data_attrs_map[name] = name
    156 

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/table/serialize.py in _construct_mixin_from_columns(new_name, obj_attrs, out)
    183 
    184     info['name'] = new_name
--> 185     col = _construct_mixin_from_obj_attrs_and_info(obj_attrs, info)
    186     out.add_column(col, index=idx)
    187 

/opt/miniconda/envs/sunpy-dev/lib/python3.6/site-packages/astropy/table/serialize.py in _construct_mixin_from_obj_attrs_and_info(obj_attrs, info)
    130     # untrusted code by only importing known astropy classes.
    131     if cls_full_name not in __construct_mixin_classes:
--> 132         raise ValueError('unsupported class for construct {}'.format(cls_full_name))
    133 
    134     mod_name, cls_name = re.match(r'(.+)\.(\w+)', cls_full_name).groups()

ValueError: unsupported class for construct sunpy.coordinates.representation.Longitude180

This enables ecsv roundtripping for SunPy coordinates.

astropy-bot · 2017-09-27T15:55:17Z

Hi there @Cadair 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issues with this pull request:

The milestone has not been set (this can only be set by a maintainer)
Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix these? Thanks!

If there are any issues with this message, please report them here

pllim · 2017-09-27T16:25:38Z

astropy/table/serialize.py

                             'astropy.coordinates.sky_coordinate.SkyCoord',
-                             'astropy.table.table.NdarrayMixin')
+                             'astropy.table.table.NdarrayMixin',
+                             'sunpy.coordinates.representation.Longitude180')


This seems confusing to me given that SunPy is not listed as optional dependency of Astropy. I feel like this should be within SunPy, not in core.

It does seem like astropy should have a public registry for these that Sunpy could add to

I don't disagree, any chance we could get this into a 2.0.x bug fix and I will add a registry for 3.0? It stops us round tripping coords to disk, which is really annoying.

Putting into 2.0.x will require a stretch of what could be called a bug fix. I guess I would label this a highly desired backport and then have a policy discussion on if / when they are allowed.

As is, I agree, but allowing subclasses serves the purposes and could be more easily argued to be a bug fix. It does mean, independently, that sunpy would only work with astropy >=2.0.3. But since it is a LTS, that may still be very useful.

mhvk · 2017-09-27T18:14:41Z

@taldcroft - what do you think? The fact this PR is needed suggests that the current setup for ecsv with a private list of classes that are OK to use does not work. I think the only real option is allowing other packages to add to it. But obviously it should be noted that in that case to read the file you need to have the corresponding package installed (or ignore all mxins).

Cadair · 2017-09-28T14:00:50Z

The other option is that I can just monkey patch this in SunPy. I.e. change the hidden list from SunPy, so if you import sunpy.coordinates suddenly the class is in the astropy list. I thought I would suggest this here first as it's a lot nicer 😉

mhvk · 2017-09-28T14:03:21Z

A possible alternative solution is to allow subclasses of anything in the current registry.

I think part of having this was for security - one doesn't want to necessarily initialize random classes - but I'm having a bit of a hard time understanding how that would work: someone can receive a file with a malicious class in it, but for opening that file to be problematic, they would also need to convince the receiver to install a package that contains that class's definition. By that point, it would seem one is surely lost anyway.

p.s. Yes, monkeypatching would solve this too, but that to me is mostly an argument for doing this right, as that means that now, effectively, this list is becoming part of the astropy API, since we would break sunpy if we moved it...

Cadair · 2017-10-04T08:55:37Z

@mhvk If someone can install a malicious class then the attack vector is not going to be opening the file, it's just going to be at install time.

Allowing subclasses would also fox this.

taldcroft · 2017-10-04T10:54:12Z

Sorry missed this issue earlier. Certainly in the initial implementation security was the concern and so having a small whitelist of classes all within Astropy was the idea. We need to think about this carefully, but at first glance I agree that it isn't obvious how one could construct an attack that way. Except of course the obvious that relies on ignorance, namely provide an ECSV data file that people want to read and put a comment that it uses blahblah package to subclass Angle and you need to install blahblah to read it. Then 95% of people will just do that without reading the code.

Will ponder.

taldcroft · 2017-10-04T11:18:29Z

I guess YAML has already "solved" this problem by having a safe_load method and big red warnings everywhere. We could do the same.

taldcroft · 2017-10-04T11:20:42Z

Nevertheless putting in place a registry of ECSV-compatible classes via the Info descriptor would probably be a good thing to allow adoption of this capability to non-astropy mixin classes.

mhvk · 2017-10-04T12:27:08Z

It would also be nice to have a fallback mechanism where if a mixin class is unknown (because, say, I haven't got sunpy installed), the conversion simply doesn't happen.

Cadair · 2017-10-15T03:56:06Z

I would like to see a fix to this reasonably quickly. Being able to use ecsv round tripping is rather useful.

another option is I PR the longitude wrap 180 class with this ;)

mhvk · 2017-10-15T15:09:53Z

another option is I PR the longitude wrap 180 class with this ;)

I think this is a good idea independently of how to allow mixin classes - in fact, we could directly use this in PhysicsSpherical. I'd argue that we could then also complement Latitude with a PolarAngle or CoLatitude or Inclination class for which angles have to be between 0 and 180 degrees (I prefer Inclination myself, since it is defined the same way for binaries - angle between orbit normal and line of sight - but perhaps CoLatitude ties it more closely to Latitude, like Longitude180 does to Longitude - hopefully, we won't get stuck in bikeshedding!).

Cadair · 2017-10-15T22:03:01Z

I will open a PR with the Longitude180 class.

Does anyone who has commented here see a way of getting a fix for this into the 2.0.z series?

Add sunpy Longitude180 to allowed mixin classes

dc27fbe

This enables ecsv roundtripping for SunPy coordinates.

pllim added the coordinates label Sep 27, 2017

pllim reviewed Sep 27, 2017

View reviewed changes

bsipocz added io.ascii table and removed coordinates labels Sep 27, 2017

astrofrog mentioned this pull request Sep 28, 2017

Use entry points to allow packages to register plugins/extensions in various parts of Astropy #6623

Open

Cadair closed this Dec 21, 2017

Uh oh!

Add sunpy Longitude180 to allowed mixin classes #6619

Add sunpy Longitude180 to allowed mixin classes #6619

Uh oh!

Conversation

Cadair commented Sep 27, 2017

Uh oh!

astropy-bot bot commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

astrofrog Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

Cadair Sep 28, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 27, 2017

Uh oh!

Cadair commented Sep 28, 2017

Uh oh!

mhvk commented Sep 28, 2017

Uh oh!

Cadair commented Oct 4, 2017

Uh oh!

taldcroft commented Oct 4, 2017

Uh oh!

taldcroft commented Oct 4, 2017

Uh oh!

taldcroft commented Oct 4, 2017

Uh oh!

mhvk commented Oct 4, 2017

Uh oh!

Cadair commented Oct 15, 2017

Uh oh!

mhvk commented Oct 15, 2017

Uh oh!

Cadair commented Oct 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astropy-bot bot commented Sep 27, 2017 •

edited

Loading