[MRG] Add CI builds for Python 3.11

[mrbean-bremen]

• change regex to be compatible with Python 3.11
• fixes Pydicom not running under Python 3.11 #1658

Describe the changes

For some reason, Python 3.11 did not like the second condition in the regex (the check for group 7 matching).
I replaced the matches with more optional groups.

Tasks

• Unit tests added that reproduce the issue or prove feature is working - n.a.
• Fix or feature added
• Code typed and mypy shows no errors
• Documentation updated (if relevant)
  • No warnings during build
  • Preview link (CircleCI -> Artifacts -> doc/_build/html/index.html)
• Unit tests passing and overall coverage the same or better

[codecov]

Codecov Report

Merging #1659 (c32358c) into master (f8cf45b) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1659      +/-   ##
==========================================
+ Coverage   97.58%   97.60%   +0.01%     
==========================================
  Files          66       66              
  Lines       10737    10737              
==========================================
+ Hits        10478    10480       +2     
+ Misses        259      257       -2     

Impacted Files	Coverage Δ
pydicom/valuerep.py	99.41% <ø> (ø)
pydicom/filebase.py	99.19% <0.00%> (+1.61%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mrbean-bremen]

I removed 3.6 here as it is EOL since last year, but I'm not sure if this is correct - did we have some plan here?

[darcymason]

I don't recall if we had a plan, but it makes sense to drop it.

[darcymason]

Shouldn't we also remove 3.6 here (and for typing)?

[mrbean-bremen]

True, will do this tonight. I had just waited for feedback...

[mrbean-bremen]

It uses an old Pypi version, will check this tonight...
BTW, removing 3.6 support also allows us to use dataclasses (as done in the example code for walk in #1696).

[mrbean-bremen]

I think this is ready to merge.

[darcymason]

I see that a couple of 3.11 parts did not pass on the master merge action.

[mrbean-bremen]

Right, I missed this! These are the tests that are not run for the PR, correct?
Looks like CharPyLS is not compiling under 3.11... maybe we shall disable the jpeg-ls tests for Python 3.11 until this is fixed, what do you think?

[darcymason]

maybe we shall disable the jpeg-ls tests for Python 3.11 until this is fixed, what do you think?

Yes, that seems very reasonable.

Interesting that CharPyLS does not install in Windows either when you look into the full log, but that section of the github actions does not show as an error; it just skips it without complaint.

[mrbean-bremen]

Interesting that CharPyLS does not install in Windows either when you look into the full log, but that section of the github actions does not show as an error; it just skips it without complaint.

I actually didn't notice this, I just saw that the test seemed to succeed - in this case I will disable it for all systems.

[mrbean-bremen]

pylibjpeg-openjpeg pylibjpeg-libjpeg also don't build with Python 3.11... at least it works with Pillow and gdcm.

[Waszker]

@mrbean-bremen I'm sorry to bring this up but could you please elaborate why this pattern works?

Looking at it I think that it's not the same as it used to be, e.g. I think that in some cases it allows for <ms> group to be matched even if the <s> one did not match. Previously this <ms> group matching was dependent on the success of matching <s> group (via the ?(7)... syntax), whereas now it's not.

Also, I wonder whether this isn't a bug in Python re module? The previous regex is correctly recognized by some other regexp implementation, e.g. https://regex101.com/

[mrbean-bremen]

I think that in some cases it allows for group to be matched even if the <s> one did not match

I don't see this - as far as I understand, the group is an optional part of the expression above, while the <s> group is mandatory (in that scope, it is optional in the scope of the <m> group). Not sure if that was clear :) Anyway, if you can show me an example that would now incorrectly match, I stand corrected, of course.

Also, I wonder whether this isn't a bug in Python re module?

Yes, it is - but I think you filed it yourself? Anywhere, it will eventually be fixed, but it is not fixed in Python 3.11.0, so we cannot remove the fix yet. In case the fix (in pydicom) is incorrect, we have to change it of course, otherwise we can just leave it as is.

[Waszker]

I think that in some cases it allows for group to be matched even if the <s> one did not match

I don't see this - as far as I understand, the group is an optional part of the expression above, while the <s> group is mandatory (in that scope, it is optional in the scope of the <m> group). Not sure if that was clear :) Anyway, if you can show me an example that would now incorrectly match, I stand corrected, of course.

Ah you're right! Somehow I didn't notice the removal of the ? on the <s> group...

Also, I wonder whether this isn't a bug in Python re module?

Yes, it is - but I think you filed it yourself? Anywhere, it will eventually be fixed, but it is not fixed in Python 3.11.0, so we cannot remove the fix yet. In case the fix (in pydicom) is incorrect, we have to change it of course, otherwise we can just leave it as is.

Yes, I have filled in a bug report once I've confirmed that there is a problem with Python's re module, but that was after posting the question here. Sorry for making it look so confusing 😅

[mrbean-bremen]

Yes, I have filled in a bug report once I've confirmed that there is a problem with Python's re module

Thanks for that, by the way, should have done this myself!

Sorry for making it look so confusing

It got me to re-check the fix, which is always a good thing. Anyway, I understand that leaving the fix in is ok, regardless of the Python re fix, so all is well.