fix ci issue #52
fix ci issue #52
Conversation
README.md
Outdated
| @@ -114,7 +114,7 @@ our [documentation](https://fidelity.github.io/seq2pat/installation.html). | |||
|
|
|||
| ### Requirements | |||
|
|
|||
| The library requires ```Python 3.7+```, the ```Cython``` package, and a ```C++``` compiler. | |||
| The library requires ```Python 3.8+```, the ```Cython``` package, and a ```C++``` compiler. | |||
There was a problem hiding this comment.
add this to changelong and bump up the version. This seems like a major change? so may be jump to 2.0.0
There was a problem hiding this comment.
add this to changelong and bump up the version. This seems like a major change? so may be jump to 2.0.0
I think maybe there is no need to add to changelog though 🤔 , thinking that we haven't changed the code, but only changed the CI test environment.
I bump up the version in docs only for making this requirement consistent to environment that can constantly pass tests.
So that is to say the library still works for python 3.7, but it is just no longer tested in CI going forward.
Do you think since this is an update in requirement, maybe it could still be a major change? Basically, we would no longer support any issues regarding seq2pat in python 3.7.
There was a problem hiding this comment.
Are we changing the Test environment OR are we changing our Requirement?
Given the tests are failing with 3.7, it seems we are changing our Requirements to 3.8+ and revising test environment accordingly. It is a major change to move from one Python version to another, no?
I would expect a note in the Changelog, since anything 1.x.x is OK with 3.7 but anything 2.x.x is not OK with 3.7.
There was a problem hiding this comment.
Are we changing the Test environment OR are we changing our Requirement?
Given the tests are failing with 3.7, it seems we are changing our Requirements to 3.8+ and revising test environment accordingly. It is a major change to move from one Python version to another, no?
I would expect a note in the Changelog, since anything 1.x.x is OK with 3.7 but anything 2.x.x is not OK with 3.7.
Thanks @skadio Yeah, we are changing both the test environment and requirement now, where they both drop 3.7 for consistency. It makes sense to update changelog and version. I will commit the changes and release new version.
There was a problem hiding this comment.
Do we know what's causing the issue? 3.7 is not that old, so I am curious what broke.
There was a problem hiding this comment.
Oh wait.. is it because GitHub Actions dropped mac support? So that means, our library actually runs on 3.7? Just that, our CI/CD tests cannot run it? But if our library can still work on 3.7, why are we jumping our requirement to 3.8? I am confused :D
There was a problem hiding this comment.
Oh wait.. is it because GitHub Actions dropped mac support? So that means, our library actually runs on 3.7? Just that, our CI/CD tests cannot run it? But if our library can still work on 3.7, why are we jumping our requirement to 3.8? I am confused :D
Yes, it is because GitHub Actions has this macos-latest image to drop 3.7, which is the image we use, then our CI test fails. To fix this, I drop 3.7 in test environment.
Then I hope to make the requirements consistent to the CI test environments, so I also change requirement to 3.8+. It is not an issue where seq2pat won't work on 3.7 though. We just won't have seq2pat tested in CI going forward.
There was a problem hiding this comment.
Oh wait.. is it because GitHub Actions dropped mac support? So that means, our library actually runs on 3.7? Just that, our CI/CD tests cannot run it? But if our library can still work on 3.7, why are we jumping our requirement to 3.8? I am confused :D
And looks like 3.7 is already end-of-life now according to this page
There was a problem hiding this comment.
I see... so one option is to upgrade CD/CI to 3.8 but leave library requirement as 3.7. And the other option is; because we cannot test it anymore, to also jump library reqs to 3.8 as well.
Agree that you can jump the library too, to be consistent with what we can test. Thank you!
There was a problem hiding this comment.
I quikcly looked at other libs. TextWiser seems most problematic at 3.6. Selective and Jurity are also 3.7 that we can bump up. Can you communicate with KArthik on Textwiser, and I will take a look at the other two.
And one syntactic comment. Can you change the text from Python 3.8+ to Python 3.8+ to bold fonts, which is the case for all other libs
README.md
Outdated
| @@ -114,7 +114,7 @@ our [documentation](https://fidelity.github.io/seq2pat/installation.html). | |||
|
|
|||
| ### Requirements | |||
|
|
|||
| The library requires ```Python 3.7+```, the ```Cython``` package, and a ```C++``` compiler. | |||
| The library requires ```Python 3.8+```, the ```Cython``` package, and a ```C++``` compiler. | |||
There was a problem hiding this comment.
Are we changing the Test environment OR are we changing our Requirement?
Given the tests are failing with 3.7, it seems we are changing our Requirements to 3.8+ and revising test environment accordingly. It is a major change to move from one Python version to another, no?
I would expect a note in the Changelog, since anything 1.x.x is OK with 3.7 but anything 2.x.x is not OK with 3.7.
README.md
Outdated
| @@ -114,7 +114,7 @@ our [documentation](https://fidelity.github.io/seq2pat/installation.html). | |||
|
|
|||
| ### Requirements | |||
|
|
|||
| The library requires ```Python 3.7+```, the ```Cython``` package, and a ```C++``` compiler. | |||
| The library requires ```Python 3.8+```, the ```Cython``` package, and a ```C++``` compiler. | |||
There was a problem hiding this comment.
Oh wait.. is it because GitHub Actions dropped mac support? So that means, our library actually runs on 3.7? Just that, our CI/CD tests cannot run it? But if our library can still work on 3.7, why are we jumping our requirement to 3.8? I am confused :D
Hi @skadio , this PR is to fix the failing CI checks. I see that CI checks failed because python 3.7 is no longer supported in runner image `macos-latest' (that is macos 14) for tests. So I think it could be ok to drop 3.7.
We can add 3.11 instead. Some tests failed in 3.12 though, due to common packages not found in the image. So 3.11 seems far enough for now.
Then I updated installation information in readme and docs, for being consistent to tests. Since these are docs changes, and no library code changes, I think we don't need to update release version or changelog.