Quickfix: Fix non-annotations on 3.7

[bryevdv]

I did not realize Python 3.7 is not currently run in CI. Many thanks to @manopapad for noticing that there were issues in 3.7 due to __future__ features unintentionally being used outside annotation contexts in a few places. The PR fixes those up, with TODOs for later. I have installed everything and run tests in a 3.7 env and will continue to develop under 3.7 for the time being. cc @magnatelee

[magnatelee]

@manopapad thanks for catching the bug. @bryevdv thanks for a quick fix! I think it's bad that we're claiming we support Python 3.7 and not actually testing with it.

@marcinz @m3vaz I believe we just pick whatever version of Python that satisfies the conda constraints for the CI image. How hard is it to run tests with more than one Python version? I think at the very minimum we should test with two versions of Python, the oldest one we're claiming to support and probably the "default" one that comes with the latest conda. Any thoughts?

[magnatelee]

A side note: #156 can make it much easier for us to test our code with multiple Python versions; we then just need to switch from one Python env to another. One caveat though is that certain features that launch Python tasks cannot be tested if the Python version Realm was compiled with is different from the interpreter's.

[manopapad]

I believe we just pick whatever version of Python that satisfies the conda constraints for the CI image.

No, currently we are explicitly asking for python 3.8 in the CI images. On my local environment I'm using 3.7, so I assume CI images with 3.7 will also work fine, but I could be wrong.

How hard is it to run tests with more than one Python version? I think at the very minimum we should test with two versions of Python, the oldest one we're claiming to support and probably the "default" one that comes with the latest conda. Any thoughts?

Given that we don't conditionally use stuff from later python versions, IMHO it should be enough to run CI on just one image, using our minimal supported python version.

[magnatelee]

No, currently we are explicitly asking for python 3.8 in the CI images. On my local environment I'm using 3.7, so I assume CI images with 3.7 will also work fine, but I could be wrong.

Then, I have to ask why we were asking for Python 3.8 and not 3.7... We should make it consistent with our minimum supported version at least.

Given that we don't conditionally use stuff from later python versions, IMHO it should be enough to run CI on just one image, using our minimal supported python version.

I want to remind you that the shutdown hang we had to fix for the previous release showed up only in the later Python versions. Issues like that might be rare enough that we don't need to worry too much, but we also don't have any control on what the Python folks decide to change down the road. So, if it's not too costly, I feel we should test at least the oldest and the latest Python versions we support.

[bryevdv]

@magnatelee all of the above considered, it also just occurs to me that the tests that would catch compatibility issues are in the cunumeric repo, not in this one. Unless I am missing something, all the CI here does is build? Is there a reason not to add a basic unit test suite here directly? Even just importing all the modules would be a start.

Edit: I see it is doing a byte compile step at the end, I’m actually not sure whether that alone would have caught this or not. I’ll look into it. Any objection to merging this now, to get others unstuck?

[magnatelee]

@bryevdv You're right. The CI for this repo only checks if the code builds correctly. I suggested @marcinz that any change to the core should also run tests for at least one of the downstream libraries, but I don't think that workflow is implemented yet. A unit test suite for Legate core sounds like a good idea. We just haven't had enough bandwidth to do all this.

Any objection to merging this now, to get others unstuck?

I have no objection to the merge.