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

ansys-api-mapdl requires protobuf<5 #3446

Open
4 tasks done
PProfizi opened this issue Oct 1, 2024 · 11 comments
Open
4 tasks done

ansys-api-mapdl requires protobuf<5 #3446

PProfizi opened this issue Oct 1, 2024 · 11 comments
Labels
bug Issue, problem or error in PyMAPDL

Comments

@PProfizi
Copy link
Contributor

PProfizi commented Oct 1, 2024

🤓 Before submitting the issue

🔍 Description of the bug

Hi all, I am raising this issue as we realized while doing integration tests that we have an issue in requirements between ansys-dpf-core and ansys-api-mapdl as can be seen here:
image

We have put a minimal requirement of protobuf>=5.27.0 for ansys-dpf-core due to issues with previous versions, namely due to the new file expected google.protobuf.runtime_version.py.

I wanted to know if there is any reason to limit ansys-api-mapdl to protobuf<5, so I know where the fix needs to be made.

Thanks

🕵️ Steps To Reproduce

The log will automatically be formatted as Python code! No need to type backticks.

💻 Which Operating System are you using?

Windows

🐍 Which Python version are you using?

3.10

💾 Which MAPDL version are you using?

latest dev

📝 PyMAPDL Report

Show the Report!

# PASTE HERE THE OUTPUT OF `python -c "from ansys.mapdl import core as pymapdl; print(pymapdl.Report())"` here

NA

📝 Installed packages

Show the installed packages!

# PASTE HERE THE OUTPUT OF `python -m pip freeze` here

📝 Logger output file

Show the logger output file.

# PASTE HERE THE CONTENT OF THE LOGGER OUTPUT FILE.

@PProfizi PProfizi added the bug Issue, problem or error in PyMAPDL label Oct 1, 2024
@PProfizi
Copy link
Contributor Author

PProfizi commented Oct 1, 2024

FYI, the error we get for ansys-dpf-core with older protobuf versions:

Python 3.10.14 (remotes/origin/f7196e89435f7282c6ea3a9901f5e664a148167e-dirty:f7196e89, Sep 15 2) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from ansys.dpf import core as dpf
>>> dpf.start_local_server(config=dpf.AvailableServerConfigs.LegacyGrpcServer)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\dpf\core\server.py", line 257, in start_local_server
    server = server_type(
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\dpf\core\server_types.py", line 1139, in __init__
    self._create_shutdown_funcs()
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\dpf\core\server_types.py", line 1155, in _create_shutdown_funcs
    self._core_api.init_data_processing_environment(self)
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\dpf\gate\errors.py", line 38, in wrapper
    out = func(*args, **kwargs)
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\dpf\gate\data_processing_grpcapi.py", line 75, in init_data_processing_environment
    from ansys.grpc.dpf import base_pb2_grpc
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\grpc\dpf\base_pb2_grpc.py", line 6, in <module>
    import ansys.grpc.dpf.base_pb2 as base__pb2
  File "C:\ANSYSDev\dpf_py_venv\lib\site-packages\ansys\grpc\dpf\base_pb2.py", line 9, in <module>
    from google.protobuf import runtime_version as _runtime_version
ImportError: cannot import name 'runtime_version' from 'google.protobuf' (C:\ANSYSDev\dpf_py_venv\lib\site-packages\google\protobuf\__init__.py)

@germa89
Copy link
Collaborator

germa89 commented Oct 1, 2024

I think there was an issue with latest 5.2X.X regarding printing a warning.... appart of that. I am not aware of any big issues.

I could give it a go in the next few weeks... unless this is super urgent.

@PProfizi
Copy link
Contributor Author

PProfizi commented Oct 2, 2024

Hey @germa89 I am about to merge this ansys/pydpf-core#1783, which does break the installation of pydpf-core along with pymapdl, thus breaking pyansys, but at least gives an understandable error.
Not sure how to handle this best.
Note that we do have ARM tests failing due to this on our side.

@PProfizi
Copy link
Contributor Author

PProfizi commented Oct 2, 2024

Hey @jorgepiloto @RobPasMue any advice on how to deal with this?

@RobPasMue
Copy link
Member

We can increase the limit to be <6 on the api-mapdl package if necessary. But the main problem @PProfizi is that setting protobuf>=5.27.0 on ansys-dpf-core as a minimum requirement is very restrictive. We just moved to protobuf 4.X to all our repos and it took more than a year... Also, AFAIK, there were some concerns raised by @greschd in regards to moving to 5.X yet (due to versions shipped internally by Ansys if I am not mistaken).

I would comment this tomorrow on the dev meeting for a larger discussion. This impacts the entire ecosystem, not only PyDPF and PyMAPDL

@greschd
Copy link
Member

greschd commented Oct 2, 2024

Also, AFAIK, there were some concerns raised by @greschd in regards to moving to 5.X yet (due to versions shipped internally by Ansys if I am not mistaken).

That was for which version to build the API packages with, not the run-time version. That version determines the lowest-supported version.

But yes, requiring >=5.x at this point all but guarantees that ansys-dpf-core won't be compatible with the other packages in the PyAnsys metapackage. We can get people to start removing that limit, but I fully expect it will again take a long time.

And it would also mean that PyDPF cannot run with the Python + packages version shipped with the unified install, which might actually be a concern in this case.

For the time being, I would suggest to keep support for 4.X, in this case probably by making (if possible) the runtime_version import optional.

@PProfizi
Copy link
Contributor Author

PProfizi commented Oct 2, 2024

Alright, thanks @greschd @RobPasMue
So I guess the actual current fix is to force a max version on the protobuf version used to build the ansys-grpc-dpf module.

@greschd
Copy link
Member

greschd commented Oct 2, 2024

So I guess the actual current fix is to force a max version on the protobuf version used to build the ansys-grpc-dpf module.

Yes, I would recommend using the same versioning scheme for building the API module as used in the ansys-tools-protoc-helper. It is outlined here: https://github.com/ansys/ansys-tools-protoc-helper?tab=readme-ov-file#grpc-version-strategy

@germa89
Copy link
Collaborator

germa89 commented Oct 2, 2024

Thank you a lot guys @greschd @RobPasMue @PProfizi

@germa89
Copy link
Collaborator

germa89 commented Oct 2, 2024

I am not closing this issue because eventually we might need to check compatibility with protobuf > 5. But we are not in a rush anymore.

@RobPasMue
Copy link
Member

Yeah, upgrading to <6 is fine in any case I'd say

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue, problem or error in PyMAPDL
Projects
None yet
Development

No branches or pull requests

4 participants