change __construct_mixin_classes to some type of registration #13689

Activity

1. nstarman

   added

   Feature Request

   on Sep 16, 2022
2. nstarman commented on Sep 17, 2022

   

   nstarman

   on Sep 17, 2022

   Last edited by nstarman

   MemberAuthor

   More actions

   With entry points we don't have to import any other package if the information is encoded in the entry point itself.
   And we can filter to only select ones from known packages, like astropy, and astropy-healpix

   [project.entry-points."table.mixins_classes"]
   CartesianRepresentation = "astropy.coordinates.representation.cartesian::CartesianRepresentation"
   HEALlPixRepresentation = "astropy-healpix...:: HEALlPixRepresentation"
   

   (I made up the HEALlPixRepresentation ).
3. taldcroft commented on Sep 19, 2022

   

   taldcroft

   on Sep 19, 2022

   Member

   More actions

   I have a custom subclass of Time that I want to be able to serialize as a trusted mixin class. Do you have an idea for how we could support that with entry points? Right now I am just inserting my custom class name into the existing list, which is certainly not ideal.
4. pllim

   added

   Refactoring

   time

   units

   coordinates

   utils.masked

   on Sep 19, 2022
5. nstarman commented on Sep 19, 2022

   

   nstarman

   on Sep 19, 2022

   Last edited by nstarman

   MemberAuthor

   More actions

   My suggestion for entry points should work for 3rd party libraries if we want to open it up, right?

   In your library's pyproject.toml add

   [project.entry-points."table.mixins_classes"]
   MyTime = "mylib.path.to.defintion::MyTime"
   

   in astropy/astropy/table/serialize.py we'd do something like (I haven't worked out the details in https://docs.python.org/3/library/importlib.metadata.html)

   def entry_point_func():
       eps = entry_points(group="table.mixins_classes")
       return tuple(".".join(ep.value.split(":")) for ep in eps)  # path.to:class -> path.to.class
   
    __construct_mixin_classes = entry_point_func()

   Notes:

   • Maybe we'd want to keep the strings as path.to:class, not path.to.class and change how table deals with the str.
   • We can add in try: Except: protections to entry_point_func
6. taldcroft commented on Sep 19, 2022

   

   taldcroft

   on Sep 19, 2022

   Last edited by taldcroft

   Member

   More actions

   And despite having expressed a desire for allowing 3rd party classes, now I'm wondering if this exposes a security risk in allowing arbitrary code to be executed if:

   • User has a malicious package installed (so it is found in entry points). Note that the package could be a secondary dependency that the user doesn't even know about.
   • Tries to read a data file that includes a mixin class from the malicious package.
   • This triggers running code from the malicious package.

   So this is a 2-factor attack requiring both a package and a data file. Maybe we could have a configuration setting to opt-in to allowing external classes?
7. mhvk commented on Sep 19, 2022

   

   mhvk

   on Sep 19, 2022

   Contributor

   More actions

   I like the idea of using entry points. To reduce the security risk, we could have a white list of packages that are allowed (just astropy by default), and warn if a class is attempted to be written or read from a non-whitelisted package (with instructions on how to fix that).

   p.s. I guess for one-off, if we keep the __construct_mixin_classes set, one can just continue to hack that.
8. taldcroft commented on Sep 19, 2022

   

   taldcroft

   on Sep 19, 2022

   Last edited by taldcroft

   Member

   More actions

   and warn if a class is attempted to be written or read from a non-allow listed package

   I'm not super-excited about a design that pretty much requires suppressing a warning. At some level maybe all we need to guard against is astropy automatically importing a 3rd party package that the user has not already imported. From a security perspective as soon as you do that import then it's game over.

   So this goes back to a configuration variable which is the user-defined allow list. Maybe with that we can automatically trust any class within the astropy namespace (i.e. all the entry points that start with astropy.)? Then my subclass module would tell astropy to trust it by adding the class to that configuration setting. Again this is taking the approach that once a user has specifically imported a package then it must be trusted because arbitrary code can run on import.
9. mhvk commented on Sep 19, 2022

   

   mhvk

   on Sep 19, 2022

   Contributor

   More actions

   Note that I wanted the warning only if a class was actually attempted to be used, so most people would never see it.

   That said, I like the idea of the user having imported a package being sufficient. Probably still needs a warning if a class is encountered that is an entry point of a package that is not yet imported.
10. taldcroft commented on Sep 19, 2022

    

    taldcroft

    on Sep 19, 2022

    Last edited by taldcroft

    Member

    More actions

    Probably still needs a warning if a class is encountered that is an entry point of a package that is not yet imported.

    I was thinking that if the class is not in the configuration allowed list then encountering that class while constructing a table would be an exception. So it either works (silently) or raises an exception. This would be independent of import. E.g.

    from astropy.table import Table, conf
    conf.mixin_class_allow_list.append('cxotime.cxotime.CxoTime')
    
    tbl = Table.read('table_with_CxoTime.ecsv')
    

    Doing from cxotime import CxoTime would probably do the same configuration update, but it would not be required if the user set the mixin_class_allow_list themselves.

    @mhvk - were you thinking something different?

    p.s. and I remembered that "allow list" and "block list" are the preferred terms now.
11. mhvk commented on Sep 19, 2022

    

    mhvk

    on Sep 19, 2022

    Contributor

    More actions

    @taldcroft - sorry, yes, not having the class in the whitelist should just raise, not warn! I guess really what I meant was that the exception message could depend on whether there is an entry point for the class.
12. nstarman commented on Sep 19, 2022

    

    nstarman

    on Sep 19, 2022

    Last edited by nstarman

    MemberAuthor

    More actions

    Just dropping in a note, https://packaging.python.org/en/latest/specifications/entry-points/#data-model

    The name of the entry point can contain dots and dashes and underscores, so we can encode information not just in the value of the entry point, but also the name. I'm thinking this is useful for backward compatibility, like when a class is moved. We can use the entry point as a mapping of the old location to the new. And then we can have pytests that will pick up on the deprecations, so they are cleaned up after an appropriate period.
13. samaloney commented on Apr 22, 2026

    

    samaloney

    on Apr 22, 2026

    Contributor

    More actions

    We just ran into something similar in sunpy, it would be handy to have a minimal way to register subclasses rather than hacking __ attributes as in sunpy/sunpy#8548
14. neutrinoceros commented on Apr 23, 2026

    

    neutrinoceros

    on Apr 23, 2026

    Contributor

    More actions

    another note from 2026 (in the midst of the rise of free-threading): any kind of global registry would require to be designed with thread safety in mind from the get go.
    I could probably take this on at some point since it looks like there is still a need for it.