Better requirements on MDF

[pgleeson]

@davidt0x @kmantel I think this is a better way to specify dependencies for mdf/modelspec. PNL doesn't explicilty use modelspec, so should leave which version to use to mdf's dependencies.

This form for the dependency will mean it will use the latest mdf 0.4.x version. If we introduce a breaking change we bump mdf to 0.5.x and updated the dependency in pnl only then.

[kmantel]

The issue is that modelspec versions may become incompatible with modeci_mdf because it's no longer pinned. This happened with modelspec 0.1.5 -> 0.2.x, which is what prompted adding the requirement here. I'm not sure this would solve that problem, unless there was some similar modelspec~= requirement on model_mdf

[davidt0x]

I would go a step further and remove the upper pin on modeci mdf entirely. This is our dependency and we should be constantly testing with the latest version to surface issues quickly.

[github-actions]

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

[kmantel]

I think that problem is handled by dependabot, which would alert us within a day that a new version is compatible or not. If we do remove or relax the requirement so that something might break, we'd be blocked from making unrelated changes until it was resolved. If there was a guarantee for no interface changes based on version number for both modeci_mdf and modelspec, then we could pin based on that.

[davidt0x]

Hey @kmantel, I understand the concern about getting blocked if there are breaking changes on mdf. However, I think setting permanent upper pins is just not the best approach for this. Why not remove the pin in general, and if such a breaking change occurs and is holding things up, then put the pin in place temporarily while its still breaking and blocking. Here is a good article on this topic by another RSE here at Princeton. I think we could also help prevent this by running the MDF PNL tests on MDF as well.

[github-actions]

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

[jvesely]

Hey @kmantel, I understand the concern about getting blocked if there are breaking changes on mdf. However, I think setting permanent upper pins is just not the best approach for this. Why not remove the pin in general, and if such a breaking change occurs and is holding things up, then put the pin in place temporarily while its still breaking and blocking. Here is a good article on this topic by another RSE here at Princeton. I think we could also help prevent this by running the MDF PNL tests on MDF as well.

There are no permanent upper pins. Dependencies are handled by an automated tool that checks whether a new version introduces new bugs or not.
There is also a difference between released/tagged versions and development version and the requirements are different. The linked article argues only about the former.
It is absolutely necessary that checking out and installing older version from git works without having to hunt for specific version of dependencies that were available when that commit was made.

At the moment the requirements*.txt files are used for both testing/development and release. Removing version constraints from them will result in random CI breakage. We had that, and it made CI barely usable.

If there is a problem with released versions a simple approach would be to add if upper_cap > latest_released: remove upper_cap to the build process of release wheels. That will give the "only cap known broken dependencies" the article argues about. There are other approaches, like introducing a separate 'constraint file', but they generally add the overhead of maintaining separate dependency lists.

Ideally, all dependencies would have both lower and upper cap and the CI would test both all max versions and all min versions (so we know when to bump the lower cap).
We don't have that, but removing version constraints from is a step in the wrong direction. Dependabot makes sure that only broken versions are restricted as most non-breaking dependencies allow the latest released version. devel ToT should never run into package constraint issues that are not caused by necessary version constraints.

[pgleeson]

I see your arguments for the current state @jvesely, but MDF is unique as a dependency here because it has PNL as a dependency itself, and usually requires the latest version. The problem arises since we need to test the development version of MDF with a recent PNL. The version will usually be incremented in dev, and not yet released, so MDF will be at, say v1.3 and any released PNL will insist on no more than 1.2, the latest released version, causing a conflict.

I do think that using ~= with the latets released version of MDF will solve almost all these issues.

[jvesely]

I see your arguments for the current state @jvesely, but MDF is unique as a dependency here because it has PNL as a dependency itself, and usually requires the latest version. The problem arises since we need to test the development version of MDF with a recent PNL. The version will usually be incremented in dev, and not yet released, so MDF will be at, say v1.3 and any released PNL will insist on no more than 1.2, the latest released version, causing a conflict.

I do think that using ~= with the latets released version of MDF will solve almost all these issues.

I don't think this conflicts with what I've said.
~=X.Y (or ~=X) should be equivalent to <X.Y+1, >=X.Y (or <X+1, >=X).

Number of version digits varies and depends on package release policies.
e.g. we currently have llvmlite<0.40, which accepts 0.39.0, or 0.39.x if there ever is a bugfix release (like there was with llvmlite-0.38.1)
On the other side of the spectrum is numpy, which occasionally changes numeric results of math operations. Since those can happen in patch releases, the cap includes all version digits.

If psyneulink needs to work with modeci_mdf-0.4.1.x, then <0.4.2, >=0.3.4 like we have now should work.
If PNL needs to work with modeci_mdf ~=0.4.x and there's a guarantee that 0.4.y won't introduce breaking changes, then <0.5, >=0.3.4 should do the job.
Note that none of those will prevent users user (pip) installing modeci_mdf==0.5 locally. pip will warn that the new package is outside of the version range, but won't prevent installation.
This allows anyone to work on future releases of both PNL, and modec_mdf without removing the safety constraints (and ideally do all the adaptations in one commit so it doesn't break bisectability)

There already is a precedent of breaking changes in 0.3.4 so removing the upper cap entirely is definitely the wrong way to go.

This is also the reason to use sharp comparison (< instead of <=). Dependabot will change <= to < on automated version bumps.
We still have few dependencies that use <= or ==, these are wrong and should be fixed, but it's low priority.

As for ~= vs. <, I don't think there's much difference. Dependabot might prefer the latter, but I haven't really checked.
if major releases of modec_mdf break API, the bump will need to be done by humans anyway (along with any modifications to adapt to the new version), so it doesn't matter if dependabot gets confused.
modeci_mdf can then be removed from automated version tracking.

[pgleeson]

I'm perfectly happy with <0.5, >=0.3.4 and we'll soon see ourselves if there are any breaking changes introduced, as we test the PNL MDF functionality every commit. If we do need to make breaking changes we'll adjust the upper/lower bound in a PR to PNL.

[jvesely]

LGTM. added @kmantel for review as well.

[github-actions]

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

[kmantel]

I don't have an issue relaxing the requirements for just breaking changes. If a similar versioning pattern holds for modelspec though, and nobody objects, I'd prefer to leave a similar cap on modelspec like modelspec<0.3, >=0.2.1.

[kmantel]

As a side note, I think the suggestion jvesely mentioned above #2459 (comment) to remove upper bounds from releases only would be worth looking into to gain the benefits of both approaches. My own hesitation is pretty much only on the development side, not the release side, at least for tentative version caps.

[pgleeson]

@kmantel Since PNL doesn't use modelspec, only through MDF, it should be up to the chosen version of MDF to specify it's requirements on modelspec, which may change more rapidly than the MDF/PNL dependence. I've added an upper cap as you suggested in the modelspec dependence in MDF here: ModECI/MDF#313.

[pgleeson]

Thanks for approving @jvesely, @kmantel. It would be great to get this merged and a new release made soon, as its preventing a new release of MDF (the only available versions of PsyNeuLink on pypi would be too restrictive).

[github-actions]

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

[jvesely]

I can merge after the CI finishes unless someone beats me to it.
@kmantel, it'd be nice if the bugfix #2467 also made it to the release.
It shouldn't take too long after this one is merged.

[kmantel]

@pgleeson new release is up