Skip to content

Conversation

@greschd
Copy link
Member

@greschd greschd commented Nov 26, 2025

Implement gRPC transport option handling for the product launcher:

  • Add dataclasses to encapsulate the transport options for each transport mode,
    with a method to instantiate the corresponding gRPC channel
  • Modify the launcher interface: for gRPC servers, the launcher must now provide
    an entry in the newly added transport_options property, instead of providing
    an entry in the urls property.
    This change is needed since the URL itself is not sufficient to create a gRPC
    channel.
  • Adapt the test and server + launcher to use UDS.

Other changes:

  • rename the plugin entry point to ansys.tools.common.launcher

@github-actions github-actions bot added documentation Improvements or additions to documentation maintenance Package and maintenance related enhancement New features or code improvements labels Nov 26, 2025
@greschd greschd force-pushed the feat/launcher-grpc-transport-options branch from 0f9f7eb to 52aa7f8 Compare November 26, 2025 16:45
@greschd greschd marked this pull request as ready for review November 27, 2025 14:54
@greschd greschd requested a review from a team as a code owner November 27, 2025 14:54
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

It's looking good - I guess this will require a new minor right?

@greschd
Copy link
Member Author

greschd commented Nov 27, 2025

I guess this will require a new minor right

Yes. I'll want to actually try using it in PyACP (currently uses the old module still) before we release, though.

@greschd greschd force-pushed the feat/launcher-grpc-transport-options branch from 42d01f7 to 2f8e4c0 Compare November 27, 2025 22:01
@greschd
Copy link
Member Author

greschd commented Nov 27, 2025

@RobPasMue what's the rationale behind creating our own exception hierarchy (specifically, the use of ProductInstanceError where it was a plain RuntimeError before) rather than using the built-in exceptions?
This is causing some problems in PyACP since I'm now catching the wrong exception type. It also means the uncaught exceptions will change type, which is technically a backwards-incompatible change to PyACP.

As such, I'm trying to figure out if PyACP should go the same route, or spend extra effort to hide this change.

@RobPasMue
Copy link
Member

@RobPasMue what's the rationale behind creating our own exception hierarchy (specifically, the use of ProductInstanceError where it was a plain RuntimeError before) rather than using the built-in exceptions? This is causing some problems in PyACP since I'm now catching the wrong exception type. It also means the uncaught exceptions will change type, which is technically a backwards-incompatible change to PyACP.

As such, I'm trying to figure out if PyACP should go the same route, or spend extra effort to hide this change.

That's a good point @greschd - the goal was to provide a set of common errors related to Ansys libraries that we could all extend from and make the experience for our users more streamlined -- they could simply catch our error and make sure that everything related to Ansys can be captured by it.. nonetheless, up for discussion. We haven't enforced it anywhere yet.

@greschd
Copy link
Member Author

greschd commented Dec 1, 2025

@RobPasMue I think the custom exceptions can make sense when there's scenario that's somewhat specific to our libraries. For example VersionError, or also the ProductInstanceError.

I'm less convinced that errors which basically mirror built-in exceptions make sense (AnsysTypeError, AnsysLogicError). Unless we want to mirror the entire exception hierarchy, we will end up with overly broad exceptions.

they could simply catch our error and make sure that everything related to Ansys can be captured by it.. nonetheless, up for discussion

Doesn't that promise indirectly imply that we'd write exception-free code / catch and re-raise all exceptions? I don't think it makes sense to raise an AnsysTypeError when we explicitly check the type, but a plain TypeError when we haven't explicitly checked, for example.

So, in short:

  • I think it makes sense to extend the built-in exception hierarchy with more specific ones, but not to duplicate what's already there.
  • I don't see a huge value in the common AnsysError base class, and would rather each exception type inherit from its most suitable built-in exception type (e.g. OSError for system-level errors, ...)

It may make sense to pull this discussion into a separate issue - for this PR I think it's okay to keep the ProductInstanceError.

@greschd greschd requested a review from RobPasMue December 1, 2025 15:32
@greschd greschd enabled auto-merge (squash) December 1, 2025 15:34
@greschd
Copy link
Member Author

greschd commented Dec 1, 2025

I guess this will require a new minor right

Yes. I'll want to actually try using it in PyACP (currently uses the old module still) before we release, though.

The test with PyACP has been successful: ansys/pyacp#921

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @greschd

@greschd greschd merged commit 4ff3fed into main Dec 1, 2025
40 checks passed
@greschd greschd deleted the feat/launcher-grpc-transport-options branch December 1, 2025 16:12
@RobPasMue
Copy link
Member

You know the changes better than us @greschd - do you want us to trigger a minor release or a patch one?

@greschd
Copy link
Member Author

greschd commented Dec 1, 2025

Minor release. Not sure if we should wait for #118

@RobPasMue
Copy link
Member

@AlejandroFernandezLuces - what's the status on #118?

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

Labels

documentation Improvements or additions to documentation enhancement New features or code improvements maintenance Package and maintenance related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants