Problem kind's versioning and Numeric kind's update

[alvalentini]

Fixing issue #435

[codecov-commenter]

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (4b06821) 85.02% compared to head (69fbdc2) 85.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
+ Coverage   85.02%   85.08%   +0.06%     
==========================================
  Files         198      200       +2     
  Lines       26073    26268     +195     
==========================================
+ Hits        22169    22351     +182     
- Misses       3904     3917      +13     

Files	Coverage Δ
...g/engines/compilers/conditional_effects_remover.py	95.55% <100.00%> (+0.20%)	⬆️
...ngines/compilers/disjunctive_conditions_remover.py	84.16% <100.00%> (+0.50%)	⬆️
...g/engines/compilers/negative_conditions_remover.py	95.92% <100.00%> (+0.15%)	⬆️
..._planning/engines/compilers/quantifiers_remover.py	93.10% <100.00%> (+0.33%)	⬆️
...fied_planning/engines/compilers/tarski_grounder.py	94.73% <100.00%> (+0.29%)	⬆️
unified_planning/engines/factory.py	74.67% <100.00%> (+0.16%)	⬆️
...ified_planning/engines/oversubscription_planner.py	91.86% <100.00%> (+0.34%)	⬆️
unified_planning/engines/plan_validator.py	92.53% <100.00%> (+0.22%)	⬆️
unified_planning/engines/replanner.py	96.00% <100.00%> (ø)
unified_planning/engines/sequential_simulator.py	88.18% <100.00%> (+0.14%)	⬆️
... and 20 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alvalentini]

@arbimo any feedback?

[arbimo]

This looks really good to me. We were able to test it in Aries and appears to work as expected.

I have only added a few comments. Note that handling the second one probably requires a bit more information, associating each feature to the version in which it was introduced. This is a suggestion and not actually sure that this is the way to go.

An additional one : I am not sure we should warn for deprecated features. In particular, to maintain backward compatibility, in Aries we are actually declaring old and new features so that thing still work if the planner is called with version 1 of the UP [1]. In essence, we declare supporting all features supported in all versions of the UP. We do this for the changes in this PR but also did this when some feature name changed pre-1.0.

[1] : https://github.com/plaans/aries/blob/8d72c7b423a6ddd15de9d31e00eaf6dc4f324b16/planning/unified/plugin/up_aries/solver.py#L45

[Framba-Luca]

Hi @arbimo ,
me, @alvalentini and @mikand discussed about this and we came up with this behavior proposal:

• When the ProblemKind's version is specified at the constructor (for example version X):

1. If some features introduced in a version > X are added to the ProblemKind -> Raise an Error
2. If some features that are deprecated in a version <= X are added -> Silent accept
3. If some features that have never been declared in the FEATURES map are added -> Raise an Error

• When the ProblemKind's version is NOT specified at the constructor:

4. When the ProblemKind.version is requested, calculate it by considering the MAX of the currently present features (like you suggested).

To do this we need to introduce a mapping from every feature to the version it was introduced and/or deprecated, as you suggested.
Our idea is to do this starting from UP version 1.0 and not before. This means that setting a feature (at theProblemKind's constructor or using the set_ method) that we removed before the release 1.0 raises an error.

This would break your current aries implementation because, to support pre 1.0 versions of the UP, you are adding features that were removed before the release 1.0.

Do you see this as a big issue or is it ok also for you to be fully retrocompatible up to 1.0?

[arbimo]

@Framba-Luca Yes that sound very reasonable.

That's OK for Aries, it only means that I will need to specify an updated minimal version for Aries in the next UP release. The risk of breakage is small (up updated but not aries) with an easy fix (just update Aries).

The upside (catching typos) is important enough for this =)

[Framba-Luca]

Hi @arbimo , I implemented the agreed modifications. As we expected, the non-updated UP-Aries version fails.

Is it OK for you to update it, so that the notebook tests will not fail and we can proceed with the merge?

[arbimo]

What is the minimum python version we require?
In Aries' CI, the test fails because of missing cache in functools (introduced in Python 3.9).

Note that python 3.8 is the default in Ubuntu 20.04 (which I use in CI), so we can still expect some people to use it.
If we have decided to require 3.9, I'll update our own requirements.

[Framba-Luca]

Thanks for noticing! No we didn't want to require 3.9, it was just a silent mistake. It should be fixed now.

[alvalentini]

@arbimo The minimum python version we require is 3.8.

[arbimo]

@Framba-Luca Thanks for fix, it works with python 3.8 now (I am surprised this wasn't caught in CI though).

I just cut a release of Aries that should be compatible with this PR and updated the minimum required version of Aries. It is a bit earlier than I anticipated and it can be expected that Aries has some surprising behavior in some numeric domains while we flush the last bugs out.

[Framba-Luca]

@arbimo I think it wasn't caught because the tests run on python 3.10 and only in master the different python versions are tested.

Thanks for the anticipated release, let's hope no one complains! :)

[arbimo]

LGTM! Thanks a lot @Framba-Luca