Deprecation procedure

[swhite2401]

Dear all,

It seems there is a need to better define deprecation and procedure to deprecate functions in pyAT.
My understanding of the present situation is:

• deprecated functions are place in the module deprecated.py
• deprecation functions are kept functional and will never be remove to keep backward compatibility
• deprecated functions will not profit for updates once the are moved to the deprecated modules

-> In effect deprecation in pyAT is not a real deprecation but rather a frozen state

I believe the main issue there is that function can be declared as deprecated with a simple PR and may go un-noticed for users not involved in the discussion since deprecation warnings are not issued to avoid overloading users scripts output.

The request from @simoneliuzzo in #601 for a more transparent procedure would deserve some thoughts. We may think the following steps:

1. function is proposed for deprecation in association with a new release instead of a simple PR
2. At this stage a warning is issued: "The function xxx will be deprecated and frozen in its present state starting from release X.X.X and will be replaced by xxx"
3. The function is still updated until deprecation
4. At the next release the function is officially deprecated, warning is removed and development stop but the function is kept functional

Please advise.

[swhite2401]

@lfarv @lnadolski @MJGaughran @carmignani @lcarver @oscarxblanco @ZeusMarti your input would be very useful!

[MJGaughran]

I agree with the need for proper policy surrounding deprecation.

I think one point of confusion is that we still haven't made a pyAT 1.0 release. In semver, the 0.x release can deprecate and remove functions at any time. Making a 1.0 release with the existing interface will give an indication that we will commit to the existing interface as we continue to develop and fix things in the near future.

I don't think indicating a function will eventually be deprecated is helpful. It is the same as our existing 'deprecation' (adding a short comment to the docstring and hiding the function), but even less clear.

I would argue for removing deprecated functions within a couple of major version increments. Even though we are still on major version 0, I think we can wait until pyAT 2.0 for any removals, since the existing deprecated functions are not problematic to maintain.

[lfarv]

I also mostly agree with the proposal of @swhite2401.

Concerning version numbers, we have not been fully compliant with semantic versioning up to now… Following that, I proposed to release a last 0.x version when the pending PRs (should be 0.4) are merged, and then move to 1.0 once the new tracking interface is formally installed.

I have however a concern about warnings: what we want is to address developers ("don't use this function any more"), while the warning will mainly affect users, who don't know what to do, apart from posting an issue. That's not what we want.
The python solution is to raise a DeprecationWarning which is ignored by default. Developers should run python in "Python Development Mode" which makes the warning appear. There is also a PendingDeprecationWarning available. But will developers do that?

Concerning removal of deprecated functions, I don't think there is a real need at the moment: once moved to a special "deprecated module", they don't need real maintenance.

[MJGaughran]

I agree that there is no real need to remove deprecated functions at the moment. My concern is in stating this as policy; people will view it as an excuse to never upgrade, and it will cause problems when we want to 'actually' deprecate more fundamental things that do cause development issues.

We should state that they can be removed after 1 or 2 major version increments, not that they actually will be.

I think using the built-in Python approach to deprecation is reasonable, unless it is commonly accepted by the wider Python community as inappropriate for use. It seems PendingDeprecationWarning isn't universally liked, in particular.

[lfarv]

Follow-up of my previous message: the python development mode may include a lot a potentially penalising features. For acting only on warnings, developers could just have PYTHONWARNINGS=default in their environment, or start python with a -Wdefault flag. Standard users would not see any warning. For those who are on both sides, it's up to them!

[swhite2401]

I agree that removing functions is not something that we would like to do unless there are very good reasons.

However, I am bit undecided about sending warnings to users or not... I remember having faced warnings in numpy and this was very useful to make the appropriate changes before upgrading to the newer version. On the other hand AT users also complained that there were too many warnings (not necessarily deprecation warnings).

There might be some cases where we would like to issue warning for the users as well, I can see in the doc that FutureWarning can be used for that, maybe this is something we could consider for critical updates (such as the tracking module)?

Other option is to keep the deprecation warnings silent for the official releases, but then activate them on the master (that is somehow the dev branch for AT)
-> the standard pip install accelerator-toolbox would not issue any warning
-> a clone from the master (or other branches) would activate warnings

I don't know if this is something easy to set up, but I suppose when can adjust the filters dynamically at import

[lfarv]

to warn or not to warn?

AT users also complained that there were too many warnings

I can understand, and the danger is that important information is hidden in the middle of a too many warnings

I would state it this way:

Nobody should get a warning about something he/she is not responsible for. I take 2 examples:

1. Someone running existing code which didn't give warnings until then should never see a deprecation warning when updating AT. Except the case where this program is known to stop working at some time in the future. We should do everything possible to avoid that, but it cannot be excluded (as you said: "for very good reasons"). In that case, the user has to stick to the previous AT release or ask the developer (possibly himself…) to upgrade the code. This is where the FutureWarning may be relevant.
2. Someone developing code based on AT should see the deprecation warnings allowing him to modify/improve his code. For this, he has to be in "developer mode". On the other hand, he should not get warnings issued by internal AT code (it's not under his responsibility). This means that no deprecated function (marked as such with a warning) may remain in internal AT code!

Of course, many people alternate between situations 1 and 2. Ideally, they should switch the developer mode depending what there are doing. Forgetting the developer mode is not dramatic: you may miss some better way to code and are more prone to a possible FutureWarning. Deprecated functions are not so numerous, and you may recover later.

Personal point of view: for me, the new tracking module would fall in the standard DeprecationWarning case (ignored be default) as long as we keep the old interface working. If some day we need to remove it, then we would mark it with a FutureWarning mentioning the release where it will be removed. But before sending the DeprecatedWarning, one must make sure it is not used any more within AT. Apparently this is done, but that's not the case for get_cells, get_refpts etc. These functions are in an intemediate "outdated" state.

[lfarv]

I looked again at the point 1 of @swhite2401's proposal:

1. function is proposed for deprecation in association with a new release instead of a simple PR

That does not fit exactly with the way development goes: a "release" is associated with a Git tag. It's a snapshot at a point where the code is considered safe, but there is no code modification corresponding to a tag. All modifications, including moving a function to a special deprecated.py module, or adding a warning, must go through the normal Pull Request review.

For me, the time at which this occurs between two releases does not matter. The deprecation will be documented in the following release notes and will be "official" at that point. In between two releases, it's "work in progress". DeprecationWarnings may appear any time once they have been reviewed, and are visible for developers identifying themselves by the "developer mode". And we keep the present guidelines: the master branch is kept safe (I don't think it has ever been broken), and it stays backward compatible all along.

We also informally said that each release will be anticipated by a pull request for advertising and discussing it, but which should not contain code modification. I am preparing the next one.

[lfarv]

Another point: I have nothing against the idea of having DeprecatedWarnings visible when installing from a GitHub clone. It would be nice indeed. But I don't know how to do that. Warning visibility is controlled when you launch Python (or programatically within the code), but not when you install… If anybody has a simple solution (I would't like a monster…)