Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/failed ci #15

Merged
merged 10 commits into from
May 30, 2024
Merged

Fix/failed ci #15

merged 10 commits into from
May 30, 2024

Conversation

kuppulur
Copy link
Contributor

This PR is to fix the following CI issue and drop support for Python 3.7.

Issue:
The scipy.linalg functions tri, triu & tril are deprecated and will be removed in SciPy 1.13.

So, updating the requirements to have a version less than 1.13.

Update requirements to fix `scipy.linalg` and `triu` import issue

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Drop 3.7 and add 3.11 support for python versions

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
@kuppulur kuppulur added the WIP label May 17, 2024
@@ -1,6 +1,7 @@
numpy
flair>=0.9
gensim>=4.0
scipy<1.13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm this is interesting.. does that mean, we can never update our scipty / go beyond 1.12?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If lingalg operations are decomissioned, what's their suggested alternative OR the right way of doing that now?

It seems to me that, there is an alternative here to actually not fixtate to sth old but to make the jump by changing how we use linalg, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is resulting from a dowstream library we use gensim. They are still in the process of releasing a fix for this, until then our best bet is to fix the scipy version. piskvorky/gensim#3525

My PR is still WIP, for some reason when I create a PR, even if I don't assign a reviewer, I guess it sends an email to you. I will get this done most probably by tomorrow and it will be casual approval for now. Thanks for the review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, thanks!

@@ -15,7 +15,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.7', '3.8', '3.9', '3.10']
python-version: ['3.8', '3.9', '3.10', '3.11']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/fidelity/textwiser?tab=readme-ov-file#installation
We stil say 3.6+, which seems inaccurate. Given that these versions of Python are decommisioned now, shall we jump to Python 3.8+? This needs changing the README and possible the Docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuppulur shall we update the docs as well? see the link above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the docs as well. Thanks for the comment.

Copy link
Contributor

@skadio skadio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some minor comments

@@ -1,6 +1,7 @@
numpy
flair>=0.9
gensim>=4.0
scipy<1.13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If lingalg operations are decomissioned, what's their suggested alternative OR the right way of doing that now?

It seems to me that, there is an alternative here to actually not fixtate to sth old but to make the jump by changing how we use linalg, no?

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Fixing Scipy to be less than 1.13 until Gensim releases a fix.

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Temporarily commented three tests which fail due to scipy version. Once the Gensim fix is released, we shall revert back.

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Dropping support for 3.6 and 3.7.

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Dropping support for 3.6 and 3.7 Python versions.

Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
Signed-off-by: Karthik Uppuluri <karthik.uppuluri@fmr.com>
@@ -15,7 +15,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.7', '3.8', '3.9', '3.10']
python-version: ['3.8', '3.9', '3.10', '3.11']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuppulur shall we update the docs as well? see the link above

@@ -1,6 +1,7 @@
numpy
flair>=0.9
gensim>=4.0
scipy<1.13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, thanks!

@kuppulur kuppulur merged commit f31a054 into master May 30, 2024
6 checks passed
@kuppulur kuppulur deleted the fix/failed_ci branch May 30, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants