API: Make fields property immutable

@tomdonaldson

This is a follow-up of:

API/BUG: only output user-requested columns in votable.parse #15959 that addressed columns parameter not working as expected in parse_single_table/parse #14943

As suggested by @tomdonaldson , "... a direct API change like making the fields property immutable (e.g., making it a sequence). Since there is no documentation mentioning the impacts of modifying fields or providing a recommendation on how modify it, starting to assume the only modifications would be through add_field() would be an indirect API change. More importantly, it would be a little presumptuous and unrealistic..."

@neutrinoceros , feel free to add details as you see fit.

Thanks, all!

Thank you for opening this. Let me try to add a bit of context:
astropy.io.votable.tree.TableElement provides an API for adding fields at runtime (namely the add_field method), however we also have tests that bypass this API and directly extend/append to the fields attribute. In #15959, and in order to implement #14943, I needed to create an explicit distinction between all available fields (all_fields) and selected fields (fields). These lists need to be kept in sync, which it made needlessly complicated if we continue to allow users to go over add_field and creates a bit of complexity that we'd like to eventually get rid of. For instance

astropy/astropy/io/votable/tree.py

Lines 2541 to 2543 in 4ab38bc

    
           for field in self._fields: 
        
               if field not in self._all_fields: 
        
                   self._all_fields.append(field)

astropy/astropy/io/votable/tree.py

Lines 2610 to 2615 in 4ab38bc

    
           new_fields = HomogeneousList( 
        
               Field, 
        
               values=[f for i, f in enumerate(self.all_fields) if i in colnumbers], 
        
           ) 
        
           if new_fields != self._fields: 
        
               self._fields = new_fields

This issue would be fixed by:

deprecating external usage of .append, .extend, and other mutating methods for the fields and all_fields attributes (which I think can be accomplished by overriding these methods in a subclass)
updating all tests that currently rely on this behavior.
opening another (last) issue to remind ourselves of everything that can be cleaned-up when the deprecation cycle ends.

I have a working proof of concept but ~~the margin's too thin~~ I had to run. I'll open a PR later today or sometimes in the weekend. As I suspected, getting everything right quickly gets quite involved.

@eerovaher

Following discussion with @eerovaher in #16316, let me reformulate the current plan for the the deprecation cycle started in that PR:

phase 1 (as astropy 6.1): TableElement.fields is a publicly mutable list
phase 2 (DEPR: deprecate direct mutations of TableElement.fields by making it an owned list #16316, aimed at astropy 7.0): TableElement.fields remains publicly mutable, but usage of mutating list APIs emits warnings
phase 3 (end of cycle, astropy N.0 where N>7): the private attribute, TableElement._fields, stays a list, but its corresponding, public-facing property TableElement.fields should now return an (immutable) tuple view of it.

This plan allows the, arguably awkward, _OwnedMixin class I created in #16316 to be removed at the end of the deprecation cycle.

Let's see what our stakeholders (e.g., PyVO) think about this plan. Thanks!

Who should we ping ?

I asked over at #pyvo channel on Astropy Slack. Feel free to ping others you think might be interested. 🤞

This plan seems reasonable to me. I wish we could be more confident nobody was relying on the mutability of fields so that this could be avoided, but I appreciate the extra care involved.

Navigation Menu

Repository navigation

API: Make `fields` property immutable #16314

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	for field in self._fields:
	if field not in self._all_fields:
	self._all_fields.append(field)

	new_fields = HomogeneousList(
	Field,
	values=[f for i, f in enumerate(self.all_fields) if i in colnumbers],
	)
	if new_fields != self._fields:
	self._fields = new_fields

API: Make fields property immutable #16314

Description

Activity

neutrinoceros commented on Apr 19, 2024

neutrinoceros commented on Apr 19, 2024

neutrinoceros commented on Apr 24, 2024

pllim commented on Apr 24, 2024

neutrinoceros commented on Apr 24, 2024

pllim commented on Apr 24, 2024

tomdonaldson commented on Apr 25, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

API: Make `fields` property immutable #16314