Support of Python 3.12 #2098
Support of Python 3.12 #2098
Conversation
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
|
let's try, this can be as simple as changing a few lines of code, or we might need to fix many dependencies.
Error in Cornac dependency: |
|
Other error, it seems it's coming from dependencies: source: https://github.com/recommenders-team/recommenders/actions/runs/9090676506/job/24984293755 |
|
As the problem is with setuptools, perhaps upgrading it or forcing a more recent version should suffice. I think cornac fails just because it is the first thing to be installed with that method. According to that issue, changing the dependency to
|
|
Now the requirements are fine, but it errors after installing build dependencies and I can't see why Source: https://github.com/recommenders-team/recommenders/actions/runs/9090676506/job/24984293755 |
|
It seems the error is the same: This seems like a long problem to debug. In my experience, it is better to debug it first in local. @daviddavo would you have time to take a look? |
|
Yeah, I can try and give it some time next week |
|
I can't install lightfm on Python 3.12, this affects recommenders too |
|
Forked the repository and made a PR The package can be installed from my repository: If the pull requests wasn't approved, I can transfer ownership of this repository to recommenders-team, and then that github repository could be used in the requirements. |
Signed-off-by: David Davó <david@ddavo.me>
|
By changing temporarily changing the requirements, it works on my machine. Let's see if it works on the test groups |
|
The following command works on my machine: But perhaps my system comes with some build dependencies that are not installed in the test groups... Tomorrow I'll try and use conda to install the package on my machine and see how it goes |
|
hey @daviddavo we were talking today in the weekly meeting about this issue. Do you have any news? |
Signed-off-by: David Davó <david@ddavo.me>
|
Ready for review just to run tests |
daviddavo
requested review from
gramhagen,
anargyri,
loomlike,
wutaomsft and
SimonYansenZhao
as code owners
|
reruning the pr gate: https://github.com/recommenders-team/recommenders/actions/runs/9747599995 |
| @@ -67,7 +67,7 @@ jobs: | |||
| strategy: | |||
| max-parallel: 50 # Usage limits: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration | |||
| matrix: | |||
| python-version: ['"python=3.8"', '"python=3.9"', '"python=3.10"', '"python=3.11"'] | |||
| python-version: ['"python=3.8"', '"python=3.9"', '"python=3.10"', '"python=3.11"', '"python=3.12"'] | |||
There was a problem hiding this comment.
I run the pr gate 3 times, and I got the same error with building the docker image:
"error": ***
"message": "Activity Failed:\n***\n \"error\": ***\n \"code\": \"UserError\",\n \"message\": \"Image build failed. For more details, check log file azureml-logs/20_image_build_log.txt.\",\n \"messageFormat\": \"Image build failed. For more details, check log file ***ArtifactPath***.\",\n \"messageParameters\": ***\n \"ArtifactPath\": \"azureml-logs/20_image_build_log.txt\"\n ***,\n \"details\": [],\n \"innerError\": ***\n \"code\": \"BadArgument\",\n \"innerError\": ***\n \"code\": \"ImageBuildFailure\"\n ***\n ***\n ***,\n \"correlation\": ***\n \"operation\": \"e4090e90a11694fd527583a50e6946eb\",\n \"request\": \"ce7a3c21a63c7552\"\n ***,\n \"environment\": \"eastus\",\n \"location\": \"eastus\",\n \"time\": \"2024-07-02T05:04:39.716946Z\",\n \"componentName\": \"RunHistory\"\n***"
Could the problem be related to #2066 @SimonYansenZhao?
There was a problem hiding this comment.
How can we access azureml-logs/20_image_build_log.txt? There are multiple reasons the BadArgument error might be raised.
Activity Failed:
***
"error": ***
"code": "UserError",
"message": "Image build failed. For more details, check log file azureml-logs/20_image_build_log.txt.",
"messageFormat": "Image build failed. For more details, check log file ***ArtifactPath***.",
"messageParameters": ***
"ArtifactPath": "azureml-logs/20_image_build_log.txt"
***,
"details": [],
"innerError": ***
"code": "BadArgument",
"innerError": ***
"code": "ImageBuildFailure"
***
***
***,
"correlation": ***
"operation": "e4090e90a11694fd527583a50e6946eb",
"request": "ce7a3c21a63c7552"
***,
"environment": "eastus",
"location": "eastus",
"time": "2024-07-02T05:04:39.716946Z",
"componentName": "RunHistory"
***
There was a problem hiding this comment.
@miguelgfierro yeah, maybe.
|
Trying again on my system it seems that the problem is related to tensorflow Tensorflow added support for 3.12 exactly in release 2.16... On of the breaking changes is the removal of tf.estimator, which is used in tf_utils.py and wide_deep_utils.py I will follow this guide: |
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
|
The build on Python 3.8 fails due to conflicting dependencies SciPy 1.13 does not support Python 3.8, but pymanopt needs Scipy 1.13 or less than 1.10, while recommenders needs 1.10 or higher. In this branch I updated pymanopt to use python 3.12, which moved some modules around, so it is not possible to use different pymanopt versions depending on the python version without convoluting the code too much. Python 3.8 security support ends on October, in 3 months. ¿Should we just remove python 3.8 testing and support? Or should we search for a set of constraints so it can be installed on python 3.8 (using scipy 1.9)? What do you think @miguelgfierro Related issues: |
|
I prefer to remove Python 3.8 |
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
.github/workflows/sarplus.yml
Outdated
| @@ -39,7 +39,7 @@ jobs: | |||
| runs-on: ubuntu-22.04 | |||
| strategy: | |||
| matrix: | |||
| python-version: ["3.8", "3.9", "3.10", "3.11"] | |||
| python-version: ["3.9", "3.10", "3.11"] | |||
| @@ -68,23 +69,19 @@ def _computeLoss_csrmatrix(a, b, cd, indices, indptr, residual_global): | |||
| residual_global[j] = num - cd[j] | |||
| return residual_global | |||
|
|
|||
| def _cost(self, params, residual_global): | |||
| def _cost(self, U, S, VT, residual_global): | |||
| """Compute the cost of GeoIMC optimization problem | |||
|
|
|||
| Args: | |||
| params (Iterator): An iterator containing the manifold point at which | |||
There was a problem hiding this comment.
Need to update the docstring too
| @@ -94,37 +91,33 @@ def _cost(self, params, residual_global): | |||
|
|
|||
| return cost | |||
|
|
|||
| def _egrad(self, params, residual_global): | |||
| def _egrad(self, U, S, VT, residual_global): | |||
| """Computes the euclidean gradient | |||
|
|
|||
| Args: | |||
| params (Iterator): An iterator containing the manifold point at which | |||
| @@ -67,7 +67,7 @@ jobs: | |||
| strategy: | |||
| max-parallel: 50 # Usage limits: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration | |||
| matrix: | |||
| python-version: ["3.8", "3.9", "3.10", "3.11"] | |||
| python-version: ["3.9", "3.10", "3.11", "3.12"] | |||
There was a problem hiding this comment.
A good way to decide whether or not to drop 3.8 is to look at the percentage of people using it. We can take a percentage and say for example, if only 5% of people use 3.8 then we drop it, otherwise, we support it. Do you know where we can find that percentage?
There was a problem hiding this comment.
This is what I got from Gemini 😃
The usage statistics of Python for websites can be found at https://w3techs.com/technologies/history_details/pl-python/3
There was a problem hiding this comment.
Lets also consideer stats for this package:
According to pypistats, its about 10% and more used than 3.11... But every day more and more deps are dropping it. You can always use an old version of this package.
Nevertheless, I can try and relax the version of numpy and see if the tests pass.
There was a problem hiding this comment.
I just found it. This is from PyPI's download statistics.
There was a problem hiding this comment.
A narrower date range gives mostly the same results.
What is certainly clear, is that there is way more people using python3.8 than python 3.12
There was a problem hiding this comment.
Ill solve the breaking changes in tensorflow and then revert the "removing 3.8" commit
There was a problem hiding this comment.
It seems now it's the second most used python. I think we should support it for now.
One way is to install some dependencies for more than 3.8 and another for less than 3.8. Like pandera here:
Line 41 in 4f86e47
Signed-off-by: David Davó <david@ddavo.me>
|
Will fail because on tensorflow 2.16:
Guides: |
This reverts commit 913abde. Signed-off-by: David Davó <david@ddavo.me>
|
Btw, do you have any method on running the tests on multiple versions on local? On the past I used tox It creates an environment for each python version you specify and installs your package there, so it also tests whether the setup.py includes everything. My current setup is having multiple environments in venv based on each python version I want to test (I just test with 3.8 and 3.12 tbh), but is prone to errors because is not automated. I don't mind adding tox: creating another branch, adding tox config there (and docu) and then merge it into this one. Some people also use tox on CI, but I think it is not necessary in this case as the flow is already established, it would be just a dev thing. |
This reverts commit 3b0d1a1. Signed-off-by: David Davó <david@ddavo.me>
- Had to do it for python 3.12 and 3.8 compat Signed-off-by: David Davó <david@ddavo.me>
|
@daviddavo my setup is also to have multiple environments. The only way to do it locally is to create a similar workflow as the Azure ml and replace the compute target with a local computer. |
|
I ended up using tox: tox.ini requires =
tox>=4
env_list = py{38,39,310,311,312}
[testenv]
description = run unit tests
extras = dev,gpu,spark
deps =
-r requirements-external.txt
commands =
python -c 'import pymanopt; print(pymanopt.__version__)'
pytest {posargs:tests/unit/recommenders} |
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
Signed-off-by: David Davó <david@ddavo.me>
|
It seems that tests are running again. Let's go back to the tensorflow problem (#2072):
I tried following the guides, but I have never used Tensorflow before, so I am having difficulties understanding what's going on, what something is supposed to do, or the convoluted structure of the project. There are lots of things in the docs that I can't find in my installed version and vice-versa. If there's someone who knows tensorflow and wants to help, I'll do a mini-guide:
recommenders/tests/unit/recommenders/utils/test_tf_utils.py Lines 35 to 36 in 8515dbf
|
Signed-off-by: David Davó <david@ddavo.me>
|
@daviddavo about this issue. If the problem is only with wide and deep. What do you think of commenting the tests and try 3.12 with the rest of the code? |
Description
Related Issues
#2097
References
Checklist:
git commit -s -m "your commit message".staging branchAND NOT TOmain branch.