Deprecation for private and semi-private callables #38211

kryzar · 2024-06-13T09:41:20Z

Problem Description

The Developer Guide, Section Deprecation does not specify if the deprecation process applies to semi-private (name starts with exactly one underscore) or private (name starts with exactly two underscores) callables (functions, methods, attributes, classes, etc).

Proposed Solution

I think the following two options are both valid:

Semi-private or private methods are for devs and are not part of the interface; users should not use them, which is a Python-wide convention, and the devs should not have to worry about keeping compatibility for semi-private and private callables.
However, it seems the developer guide deprecation process applies to those as well, and not all users are aware of the _ and __ convention.

Both options are fine to me and I believe this is a minor issue. However, I think the community should pick a single approach and make it somewhat official but explicitly putting it in the developer guide.

Alternatives Considered

See above.

Additional Information

No response

Is there an existing issue for this?

I have searched the existing issues for a bug report that matches the one I want to file, without success.

The text was updated successfully, but these errors were encountered:

grhkm21 · 2024-06-16T23:25:12Z

@mkoeppe @tscrim @kwankyu Pinging you three because of the discussion on my PR, also about deprecating things.

Travis has commented in the past on my other PR that semi-private and private methods indeed do not go through the deprecation process, but other developers are saying I'm cherry-picking comments, so writing this down somewhere concrete would be better.

kwankyu · 2024-06-17T05:43:54Z

To avoid further confusion, let's try to settle down this issue. Continuing the discussion from #38207, I suggest:

The deprecation policy means what is described in https://doc.sagemath.org/html/en/developer/coding_in_python.html#deprecation.

We want to clarify the policy with the following details:

(S) Only the definitions of functions/methods/attributes/classes living under sage. are subject to the deprecation policy.
(M) A function/method/attribute whose name starts with one or more underscores are considered internal, and is not subject to the deprecation policy.
(C) A class whose name has an underscore anywhere is internal, and is not subject to the deprecation policy.
(E) Experimental code following https://doc.sagemath.org/html/en/developer/coding_in_python.html#experimental-unstable-code is not subject to the deprecation policy.

More suggestions? Missing cases?

kryzar · 2024-06-18T14:39:21Z

~~Sounds good to me.~~ Maybe some examples could be provided in the dev. guide.

EDIT: Stroke my comment after the next two comments.

roed314 · 2024-06-18T18:32:21Z

I worry that (C) is too permissive. For example, NumberField_quadratic has an underscore; I think it's fairly common in the Sage codebase to use underscores to divide sections of a name.

Is there an example of a class that you think should be considered internal?

dimpase · 2024-06-18T18:51:08Z

I worry that (C) is too permissive. For example, NumberField_quadratic has an underscore; I think it's fairly common in the Sage codebase to use underscores to divide sections of a name.

indeed, (C) should not happen.

Is there an example of a class that you think should be considered internal?

kwankyu · 2024-06-18T19:28:24Z

For example, NumberField_quadratic has an underscore; I think it's fairly common in the Sage codebase to use underscores to divide sections of a name.

Right. Then do you think that NumberField_quadratic is in public API? So that if you would change NumberField_quadratic to NumberField_quadratic_extension, NumberField_quadratic should be subject to the deprecation policy?

If you think so, would you suggest a replacement of (C)?

Or do you think any class name (starting with a letter other than an underscore) change should be subject to the deprecation policy?

mkoeppe · 2024-06-18T19:37:04Z

For example, NumberField_quadratic has an underscore;

Yes, this is a good example for this discussion. NumberField_quadratic an implementation class, and it is not necessary to have it in the public API.

The public API to create a number field is through the sage.rings.number_field.number_field.NumberField factory function.
And a way to check whether an object K is a quadratic number field using only public API is to use isinstance(K, sage.rings.number_field.number_field_base.NumberField) and K.base_field() == QQ and K.degree() == 2.

roed314 · 2024-06-18T19:51:04Z

Does the deprecation policy apply only to renaming the class, or also to changing methods on it? I agree that it's probably fine to rename NumberField_quadratic to NumberField_quadratic_extension; it's not a good idea to be able to modify methods on it at will just because this class is considered "internal."

mkoeppe · 2024-06-18T20:01:08Z

Users should certainly be able to rely on the interface of public methods of objects that have been constructed using public API.

saraedum · 2024-06-18T20:22:36Z

I'm not opposed to the proposal but just for the record, these classes are in sage.all and contain an underscore:

['Berkovich_Cp_Affine',
 'Berkovich_Cp_Projective',
 'DynamicalSemigroup_affine',
 'DynamicalSemigroup_projective',
 'DynamicalSystem_Berkovich',
 'DynamicalSystem_affine',
 'DynamicalSystem_product_projective',
 'DynamicalSystem_projective',
 'Euler_Phi',
 'GroupExp_Class',
 'PermutationGroupMorphism_id',
 'PermutationGroupMorphism_im_gens',
 'PermutationGroup_generic',
 'PermutationGroup_subgroup',
 'Riemann_Map']

kwankyu · 2024-06-18T20:29:49Z

If we adopt (C), then this gives a clear reason to choose either one of QuadraticNumberField (public) and NumberField_quadratic (internal) to name a class implementing quadratic number fields.

Moreover after adopting (C), if one wants to make a public version out of NumberField_quadratic, then he can just rename it to QuadraticNumberField without deprecation warning :-)

kwankyu · 2024-06-18T20:34:38Z

I'm not opposed to the proposal but just for the record, these classes are in sage.all and contain an underscore:

['Berkovich_Cp_Affine',
 'Berkovich_Cp_Projective',
 'DynamicalSemigroup_affine',
 'DynamicalSemigroup_projective',
 'DynamicalSystem_Berkovich',
 'DynamicalSystem_affine',
 'DynamicalSystem_product_projective',
 'DynamicalSystem_projective',
 'Euler_Phi',
 'GroupExp_Class',
 'PermutationGroupMorphism_id',
 'PermutationGroupMorphism_im_gens',
 'PermutationGroup_generic',
 'PermutationGroup_subgroup',
 'Riemann_Map']

We should regard these as exceptions to the rule (C) initially, and sort them our case by case basis with deprecation warnings. Thanks for the list.

roed314 · 2024-06-18T20:43:08Z

How about

(C) A class that is not imported into the global namespace and whose name has an underscore anywhere is internal, and is not subject to the deprecation policy. This means that the name of the class can be changed, but methods on the class may still be subject to deprecation if instances of the class are returned by Sage's public API.

kwankyu · 2024-06-18T20:53:08Z

Does the deprecation policy apply only to renaming the class, or also to changing methods on it?

If (C) applies to a class, the class is internal, and anything with it can be done without deprecation warning, including even removing the class entirely as we refactor code.

As Matthias put it, public methods of the class accessible by public API should still be accessible via other class by public API.

mkoeppe · 2024-06-18T21:15:46Z

Regarding deprecation of things in the public namespace, see

Meta-ticket: deprecate useless or misleading functions in the global namespace #25383

mkoeppe · 2024-06-18T21:18:13Z

Another relevant pointer: https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies on the "modularization anti-pattern" isinstance(..., SomeImplementation_class)

mkoeppe · 2024-06-18T21:44:38Z

'Euler_Phi',

This one was recently garbage-collected

tscrim · 2024-06-19T01:01:46Z

Any class that is in our public documentation is subject to be imported and used in a user's code (such as my own that I have/will not make a PR for). So (C) or any variation thereof is a poor option as it is very likely to break code in the wild (without any deprecation message); especially as we are moving to doing isinstance checks in our code base (which people use for reference in writing their own).

I think (ME) is the only sensible thing to do. If a user is really using hidden functions, they clearly know what they are doing and understand the risks that it might change without notice. Experimental code explicitly says it can change without notice.

mkoeppe · 2024-06-19T01:08:34Z

[...] especially as we are moving to doing isinstance checks

We aren't moving to doing isinstance checks with random implementation classes. We are moving to doing isinstance checks with advertised abstract base classes.

tscrim · 2024-06-19T01:13:21Z

[...] especially as we are moving to doing isinstance checks

We aren't moving to doing isinstance checks with random implementation classes. We are moving to doing isinstance checks with advertised abstract base classes.

Sorry, I was not being precise enough. Yes, we are moving towards that and away from is_* functions. The key word is advertised, which to me means in the public documentation.

mkoeppe · 2024-06-19T01:14:53Z

Of course these ABCs are in the public documentation.

mkoeppe · 2024-06-19T01:16:28Z

What does not check out is the other direction. Just because an implementation class is documented, it does not mean that it is part of the public API.

saraedum · 2024-06-19T01:32:36Z

Any class that is in our public documentation is subject to be imported and used in a user's code

I don't think I agree with this argument. A _method() is also in our documentation and still we do not tell people that they can rely on its API I think. I would want to use classes as implementation details but if all classes are part of the stable API then we only have _method() to hide implementation details?

Is this what you had in mind?

tscrim · 2024-06-19T01:58:27Z

Any class that is in our public documentation is subject to be imported and used in a user's code

I don't think I agree with this argument. A _method() is also in our documentation

How are you defining documentation here? In a strict reading, whereby it is a method/function/etc. with a docstring, then yes. Yet, my point is about what is public, and to me, that is what appears in our reference manual as we say that is the official (public) SageMath documentation. That is what you typically find by Google, what I tell people to look at, etc.

and still we do not tell people that they can rely on its API I think. I would want to use classes as implementation details but if all classes are part of the stable API then we only have _method() to hide implementation details?

If you want to hide it, then make it a _HiddenClass. If you are going to split our a computational part into a class, then shouldn't users generally have access to that to use it as they see fit?

Is this what you had in mind?

Does that clarify? Of course, we don't have to add a particular .py to the reference manual too, which would make it hidden. Although I think that needs a very high level of justification, but that is a digression.

mkoeppe · 2024-06-19T02:02:01Z

my point is about what is public, and to me, that is what appears in our reference manual as we say that is the official (public) SageMath documentation. That is what you typically find by Google, what I tell people to look at, etc.

That's much too rigid. Our documentation is written for a wide audience, not just for an audience that is looking for stable public API.

kwankyu · 2024-06-19T02:09:30Z

According to the documentation,

class name in `sage.all`	intended to be	renaming suggested by me
`Berkovich_Cp_Affine`	public	`BerkovichCpAffine`
`Berkovich_Cp_Projective`	public	`BerkovichCpProjective`
`DynamicalSemigroup_affine`	public	`DynamicalSemigroupAffine`
`DynamicalSemigroup_projective`	public	`DynamicalSemigroupProjective`
`DynamicalSystem_Berkovich`	public	`DynamicalSystemBerkovich`
`DynamicalSystem_affine`	public	`DynamicalSystemAffine`
`DynamicalSystem_product_projective`	internal, accessible via `DynamicalSystem_projective`
`DynamicalSystem_projective`	public	`DynamicalSystemProjective`
`Euler_Phi`	internal, accessible via `euler_phi`	#30322
`GroupExp_Class`	internal, accessible via `GroupExp`	#38238
`PermutationGroupMorphism_id`	internal, accessible via `PermutationGroupMorphism`
`PermutationGroupMorphism_im_gens`	public	combined to `PermutationGroupMorphism`
`PermutationGroup_generic`	internal, accessible via `PermutationGroup`
`PermutationGroup_subgroup`	internal, accessible via subgroup method of `PermutationGroup`
`Riemann_Map`	public	`RiemannMap`

mkoeppe · 2024-06-19T21:57:29Z

Would (RNC) and (RMC) in #38211 (comment) be solutions to deprecate renamed or removed public (not internal) class?

Any solution using lazy_import only works for isinstance, but not for subclassing, as I explained in #38207 (comment)

kwankyu · 2024-06-19T22:00:08Z

Would subclassing work with imported lazy_import object?

mkoeppe · 2024-06-19T22:01:01Z

No, because a LazyImport instance is not even a subclass of type.

kwankyu · 2024-06-19T22:02:45Z

So still (RNC) will break the user code... This is better than no deprecation message. ?

mkoeppe · 2024-06-19T22:05:16Z

The primary purpose of deprecation is compatibility, not sending messages.

It will again depend on common sense / domain expertise to determine whether compatibility for subclassing should be sacrificed just for issuing a deprecation message.

kwankyu · 2024-06-19T22:13:13Z

For a removed (rather to be removd) public class, (RMC) would work? But this is a rare (if ever) case...

kwankyu · 2024-06-19T22:19:44Z

For a removed (rather to be removd) public class, (RMC) would work? But this is a rare (if ever) case...

Again no. Would not work for subclassing...

kwankyu · 2024-06-19T22:27:49Z

The primary purpose of deprecation is compatibility, not sending messages.

The primary purpose is rather sending message while keeping compatibility. The intention is to notify the user with what would happen before it happens. "Incompatibility and message" seems better than "compatibility and no message".

and I think (RNC) is a good solution for all other cases.

mkoeppe · 2024-06-19T22:30:15Z

"Incompatibility and message" seems better than "compatibility and no message".

Absolutely not.

kwankyu · 2024-06-19T22:33:03Z

"compatibility and no message" is just a delayed break (with surprise) of user code. No?

mkoeppe · 2024-06-19T22:35:56Z

That's right. Delaying a breaking change is the point of deprecation. The delay gives people time to act.

tscrim · 2024-06-19T22:37:57Z

The point of the deprecation is to emit a message so users are not surprised when it does go away. Likewise, if it is incompatible (which can be possible) but it does give a message, then that can make clear to the user that they need to change something at that time.

mkoeppe · 2024-06-19T22:41:44Z

Likewise, if it is incompatible (which can be possible) but it does give a message, then that can make clear to the user that they need to change something at that time.

That's not deprecation, that's an error message.

kwankyu · 2024-06-19T22:44:36Z

That's right. Delaying a breaking change is the point of deprecation. The delay gives people time to act.

If people is not notified somehow, what is the use of the delay?

mkoeppe · 2024-06-19T22:47:11Z

Deprecation messages are just one part of a communication strategy. Downstream developers stay informed about the projects that they use in various ways.

tscrim · 2024-06-19T22:55:52Z

Likewise, if it is incompatible (which can be possible) but it does give a message, then that can make clear to the user that they need to change something at that time.

That's not deprecation, that's an error message.

Not in general. Plus, you need to be able to set the message, which you cannot do with, e.g., an ImportError.

kwankyu · 2024-06-19T22:56:44Z

It is getting vague...

If "compatibility and (in-code) message" is not possible (as in the subclassing case), then some message (communication) should be posted somewhere at least. We may post these in the release note. I think (hope ?) that these deprecations of public classes are rare events. So this way of communication would not be burdensome...

mkoeppe · 2024-06-19T23:01:10Z

If "compatibility and (in-code) message" is not possible (as in the subclassing case), then some message (communication) should be posted somewhere at least. We may post these in the release note.

Well, deprecations should be noted already in the title of the PR that deprecates it, which will make it show up in the automatically generated release notes: https://github.com/sagemath/sage/releases/tag/10.1

Having a section on deprecations in release notes is a very standard practice. See for example https://docs.scipy.org/doc/scipy/release/1.13.0-notes.html#deprecated-features

mkoeppe · 2024-06-19T23:03:41Z

Also https://github.com/sagemath/sage/wiki/Sage-10.4-Release-Tour#deprecations

kwankyu · 2024-06-19T23:06:10Z

OK. It seems that posting deprecations of public classes in the "Deprecation" section of the release notes is the best we can do. So deprecation of public classes is done differently from the deprecation policy of functions/methods/attributes.

mkoeppe · 2024-06-19T23:07:52Z

Note that also for some aspects of our Cython interfaces we do not have any deprecation infrastructure other than code comments and release notes.

(Edit: someone may have mentioned that already somewhere above)

kwankyu · 2024-06-19T23:14:32Z

Back to "public and internal" discussion,

we now have, as alternatives:

(C) A class is internal if its name has an underscore anywhere is internal

(CT) = (M') A function/method/attribute/class/etc. whose name starts with one or more underscores are considered internal or does not appear in the public documentation (the published html https://doc.sagemath.org/html/en/ or the corresponding pdf), and is not subject to the deprecation policy.

(CL) A class is internal if its name starts with an underline or it cannot be importable from command line

(CS) A class is internal if experts (reviewers for a PR) think so. If experts disagree, then it is public.

People here give their preferences or suggest modifications or a new alternative.

I will collect a couple of most preferred alternatives, and bring the discussion to sage-devel.

saraedum · 2024-06-19T23:29:04Z

Thanks for compiling the list. I like (CL) + (CS).

roed314 · 2024-06-19T23:43:50Z

I agree with (CL) + (CS). We could elaborate slightly on (CS) by saying
(CS') A class is internal if experts (reviewers for a PR) agree that it is unlikely to be directly imported by user code. If experts disagree, then it is public.

grhkm21 · 2024-06-19T23:57:54Z

Is my understanding correct that (CT) = (M') is a subset of (CL)?

Anyways, I am +1 with (CT) + (CL) + (CS)/(CS'). FWIW (CS) has been the case in the past

kwankyu · 2024-06-20T01:01:27Z

Is my understanding correct that (CT) = (M') is a subset of (CL)?

Actually as sets, probably they are the same, unless there is some module that is omitted in the documentation (perhaps by mistake).

mezzarobba · 2024-06-20T08:39:31Z

It seems that posting deprecations of public classes in the "Deprecation" section of the release notes is the best we can do. So deprecation of public classes is done differently from the deprecation policy of functions/methods/attributes.

Regarding this point, it seems to me that for public classes that are not part of the global namespace, the best option is often to keep the old name as an alias forever.

kwankyu · 2024-06-20T10:10:26Z

We may define the following standard procedures, from practices.

(RNP) Procedure to rename a public class
(RNP step 1) rename class OldClass to class NewClass, add alias OldClass = NewClass:

class NewClass:
    ....

OldClass = NewClass   # OldClass is deprecated. See Issue 12345.

(RNP step 2) before the next stable release, post the change to the "Deprecated" section of the release note.

The alias will be there forever unless someone steps up to remove it :-)

(RMP) Procedure to remove a public class
(RMP step 1) add a comment

# Class is deprecated. See Issue 12345.

class Class:

(RMP step 2) before the next stable release, post the change to the "Deprecated" section of the release note.

kwankyu · 2024-06-20T10:53:45Z

I think we are already converging to a consensus. So I will bring only the following items to sage-devel for wider discussion:

(S) Only public functions, methods, attributes, classes living under sage are subject to the deprecation policy.

(M) Functions, methods, attributes, classes whose names start with an underscore are considered internal, and is not subject to the deprecation policy.

(C) A class whose name does not start with an underscore is public by default, but can be considered internal if experts agree that the class is unlikely to be directly imported by user code. If experts disagree, then it is public. For a PR changing the class, authors and reviewers are the experts.

@saraedum

…xpElement`, `GroupSemidirectProductElement`       `GroupExp_Class` as discussed in: - sagemath#38211 (comment) @saraedum Part of: - sagemath#25383 ### 📝 Checklist  - [x] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#38238 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee

kryzar added the t: enhancement label Jun 13, 2024

kryzar mentioned this issue Jun 13, 2024

Delete _check_rank_two method for Drinfeld modules #38199

Merged

fchapoton changed the title ~~Depreciation for private and semi-private callables~~ Deprecation for private and semi-private callables Jun 14, 2024

mkoeppe added the c: documentation label Jun 16, 2024

Deprecation for private and semi-private callables #38211

Deprecation for private and semi-private callables #38211

Comments

kryzar commented Jun 13, 2024 • edited by kwankyu Loading

Problem Description

Proposed Solution

Alternatives Considered

Additional Information

Is there an existing issue for this?

grhkm21 commented Jun 16, 2024

kwankyu commented Jun 17, 2024 • edited Loading

kryzar commented Jun 18, 2024 • edited Loading

roed314 commented Jun 18, 2024

dimpase commented Jun 18, 2024

kwankyu commented Jun 18, 2024

mkoeppe commented Jun 18, 2024 • edited Loading

roed314 commented Jun 18, 2024

mkoeppe commented Jun 18, 2024

saraedum commented Jun 18, 2024 • edited Loading

kwankyu commented Jun 18, 2024 • edited Loading

kwankyu commented Jun 18, 2024 • edited Loading

roed314 commented Jun 18, 2024

kwankyu commented Jun 18, 2024

mkoeppe commented Jun 18, 2024

mkoeppe commented Jun 18, 2024

mkoeppe commented Jun 18, 2024

tscrim commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

tscrim commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

saraedum commented Jun 19, 2024

tscrim commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024 • edited Loading

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024 • edited Loading

kwankyu commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

tscrim commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

tscrim commented Jun 19, 2024

kwankyu commented Jun 19, 2024 • edited Loading

mkoeppe commented Jun 19, 2024

mkoeppe commented Jun 19, 2024

kwankyu commented Jun 19, 2024

mkoeppe commented Jun 19, 2024 • edited Loading

kwankyu commented Jun 19, 2024

saraedum commented Jun 19, 2024

roed314 commented Jun 19, 2024

grhkm21 commented Jun 19, 2024 • edited Loading

kwankyu commented Jun 20, 2024

mezzarobba commented Jun 20, 2024

kwankyu commented Jun 20, 2024

kwankyu commented Jun 20, 2024

kryzar commented Jun 13, 2024 •

edited by kwankyu

Loading

kwankyu commented Jun 17, 2024 •

edited

Loading

kryzar commented Jun 18, 2024 •

edited

Loading

mkoeppe commented Jun 18, 2024 •

edited

Loading

saraedum commented Jun 18, 2024 •

edited

Loading

kwankyu commented Jun 18, 2024 •

edited

Loading

kwankyu commented Jun 18, 2024 •

edited

Loading

kwankyu commented Jun 19, 2024 •

edited

Loading

kwankyu commented Jun 19, 2024 •

edited

Loading

kwankyu commented Jun 19, 2024 •

edited

Loading

mkoeppe commented Jun 19, 2024 •

edited

Loading

grhkm21 commented Jun 19, 2024 •

edited

Loading