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: added check for FriCAS version #39796

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

devansh-srv
Copy link

@devansh-srv devansh-srv commented Mar 26, 2025

This pull request includes several updates to the FriCAS class in the src/sage/features/fricas.py file. The changes introduce a minimum version requirement, add a method to retrieve the installed FriCAS version, and enhance the is_functional method to check the version.This is an attempt to fix the issue 39784

Enhancements to version handling and functionality checks:

  • src/sage/features/fricas.py: Added a MINIMUM_VERSION attribute to specify the minimum required version of FriCAS.
  • src/sage/features/fricas.py: Introduced a get_version method to retrieve the installed FriCAS version.
  • src/sage/features/fricas.py: Updated the is_functional method to call get_version and check if the installed version meets the minimum requirement. If the version is not retrievable or is too old, appropriate error messages are returned.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@devansh-srv
Copy link
Author

Hey @dimpase
I have checked that the url https://fricas.github.io/ refers to version 1.3.11
but this ticket concludes that FriCAS support was updated to 1.3.8
so I think 1.3.8 version check would be appropriate but I need to be sure
I am not on a Ubuntu machine (rather an arch machine)

@dimpase
Copy link
Member

dimpase commented Mar 26, 2025

indeed, anything earlier than 1.3.8 wasn't supported. Perhaps some tests, updated for newer FriCAS, might now fail, but that's OK

@tscrim
Copy link
Collaborator

tscrim commented Mar 28, 2025

cc-ing @mantepse just in case

Copy link

github-actions bot commented Mar 28, 2025

Documentation preview for this PR (built with commit 9897efb; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@dimpase
Copy link
Member

dimpase commented Mar 29, 2025

can we use a non-deprecated API here?

>>> from pkg_resources import parse_version
<stdin>:1: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html

@dimpase
Copy link
Member

dimpase commented Mar 29, 2025

e.g. you can use https://packaging.pypa.io/en/stable/version.html

@devansh-srv
Copy link
Author

e.g. you can use https://packaging.pypa.io/en/stable/version.html

We can but is it causing any issue in the building process?

pkg_resources

@dimpase the package pkg_resources definitely needs to go as it is deprecated as shown by pytorch ticket 139170
I have made the necessary changes using the packaging.version package
I have also written and optional EXAMPLE doctest for the get_version() method
you may have a look on these latest changes

@devansh-srv
Copy link
Author

Please review the changes @dimpase

@dimpase
Copy link
Member

dimpase commented Apr 1, 2025

e.g. you can use https://packaging.pypa.io/en/stable/version.html

We can but is it causing any issue in the building process?

Deprecated features should not be used in newly written code, this is the only reason for my request

pkg_resources

@dimpase the package pkg_resources definitely needs to go as it is deprecated as shown by pytorch ticket 139170

There are more uses of it across Sage code,
which need fixing before we can remove it

I have made the necessary changes using the packaging.version package
I have also written and optional EXAMPLE doctest for the get_version() method
you may have a look on these latest changes

@devansh-srv
Copy link
Author

e.g. you can use https://packaging.pypa.io/en/stable/version.html

We can but is it causing any issue in the building process?

Deprecated features should not be used in newly written code, this is the only reason for my request

pkg_resources

@dimpase the package pkg_resources definitely needs to go as it is deprecated as shown by pytorch ticket 139170

There are more uses of it across Sage code, which need fixing before we can remove it

I have made the necessary changes using the packaging.version package
I have also written and optional EXAMPLE doctest for the get_version() method
you may have a look on these latest changes

In the latest changes I have remove pkg_resources and use the package that you have suggested
I think this should be fine

@devansh-srv
Copy link
Author

Hey @dimpase any specific instructions for me over this PR ?
Just needed to know if I need to change something

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

your test for fricas version is too explicit. As we allow external fricas, it could be another version.

All you know is that's it's 1.3.something.

please replace 8 with dots, as done in such cases

@devansh-srv devansh-srv requested a review from dimpase April 4, 2025 05:14
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

Please add the following comment

--- a/build/pkgs/fricas/spkg-configure.m4
+++ b/build/pkgs/fricas/spkg-configure.m4
@@ -1,4 +1,7 @@
 SAGE_SPKG_CONFIGURE(
+dnl
+dnl make sure that the minimal version is also set in src/sage/feature/fricas.py
+dnl
     [fricas], [
         AC_CACHE_CHECK([for FriCAS >= 1.3.8], [ac_cv_path_FRICAS], [
         AC_PATH_PROGS_FEATURE_CHECK([FRICAS], [fricas], [

Otherwise it's good to go.

@roed314 - please turn this to Positive review

@devansh-srv
Copy link
Author

Please add the following comment

--- a/build/pkgs/fricas/spkg-configure.m4
+++ b/build/pkgs/fricas/spkg-configure.m4
@@ -1,4 +1,7 @@
 SAGE_SPKG_CONFIGURE(
+dnl
+dnl make sure that the minimal version is also set in src/sage/feature/fricas.py
+dnl
     [fricas], [
         AC_CACHE_CHECK([for FriCAS >= 1.3.8], [ac_cv_path_FRICAS], [
         AC_PATH_PROGS_FEATURE_CHECK([FRICAS], [fricas], [

Otherwise it's good to go.

@roed314 - please turn this to Positive review

shall I do these changes now?

@dimpase
Copy link
Member

dimpase commented Apr 4, 2025

yes, of course.
At the moment we don't have a clear way to make sure there is only one source for the minimal version of an optional package like fricas,
so the purpose of this comment is to make sure that in the future these versions don't get out of sync.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 5, 2025
sagemathgh-39796: fix: added check for FriCAS version
    
This pull request includes several updates to the `FriCAS` class in the
`src/sage/features/fricas.py` file. The changes introduce a minimum
version requirement, add a method to retrieve the installed FriCAS
version, and enhance the `is_functional` method to check the
version.This is an attempt to fix the issue
[39784](sagemath#39784)

Enhancements to version handling and functionality checks:

* [`src/sage/features/fricas.py`](diffhunk://#diff-
a2c0d819d857ba2338e0d58e1a4875c88b600b174c8fb4371183ecc3f0e5b72eR29-
R30): Added a `MINIMUM_VERSION` attribute to specify the minimum
required version of FriCAS.
* [`src/sage/features/fricas.py`](diffhunk://#diff-
a2c0d819d857ba2338e0d58e1a4875c88b600b174c8fb4371183ecc3f0e5b72eR43-
R54): Introduced a `get_version` method to retrieve the installed FriCAS
version.
* [`src/sage/features/fricas.py`](diffhunk://#diff-
a2c0d819d857ba2338e0d58e1a4875c88b600b174c8fb4371183ecc3f0e5b72eL61-
R85): Updated the `is_functional` method to call `get_version` and check
if the installed version meets the minimum requirement. If the version
is not retrievable or is too old, appropriate error messages are
returned.
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39796
Reported by: Devansh Srivastava
Reviewer(s): Dima Pasechnik
@devansh-srv devansh-srv requested a review from dimpase April 6, 2025 16:52
@devansh-srv
Copy link
Author

@dimpase I have made the suggested changes in the file /build/pkgs/fricas/spkg-configure.m4
Have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants