ansys-api-mapdl requires protobuf<5

PProfizi commented 2 weeks ago

🤓 Before submitting the issue

[X] I have visited the Troubleshooting section.
[X] I have searched among the existing issues.
[X] I am using a Python virtual environment.
[X] I am using the latest version of PyMAPDL and its dependencies (fully updated virtual environment). You can update them using pip install --upgrade --upgrade-strategy eager ansys-mapdl-core in your activated virtual environment.

🔍 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:

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!

```text # 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!

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

📝 Logger output file

Show the logger output file.

```text # PASTE HERE THE CONTENT OF THE LOGGER OUTPUT FILE. ```

PProfizi commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Hey @germa89 I am about to merge this https://github.com/ansys/pydpf-core/pull/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 commented 2 weeks ago

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

RobPasMue commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Thank you a lot guys @greschd @RobPasMue @PProfizi

germa89 commented 2 weeks ago

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 commented 2 weeks ago

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

ansys / pymapdl