chore: v1 implementation of repair stubs #2441
Conversation
V1 implementation of repair tools. Notes 1) The return argument from the tracker were inconsistent between calls and were bringing outside in the service layer historical names (tracker_response vs complete_command_response) that is better to keep into v0. Some small changes were made to the v0 to eliminate the problem 2) The return argument of functions (repair and others) that modify data in an unknown way needs to be made consistent but we can tackle that topic separately from v1 creation 3) More stuff was put into conversions.py. Definitely needs a clean-up...
smereu
requested review from
RobPasMue,
b-matteo,
david-gorman and
jacobrkerstetter
github-actions
bot
added
maintenance
github-actions
bot
added
maintenance
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2441 +/- ##
==========================================
- Coverage 91.63% 90.64% -0.99%
==========================================
Files 169 169
Lines 11841 12030 +189
==========================================
+ Hits 10850 10905 +55
- Misses 991 1125 +134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
maintenance
github-actions
bot
added
maintenance
jacobrkerstetter
left a comment
jacobrkerstetter
left a comment
There was a problem hiding this comment.
Make sure you're testing by forcing to v1 for this stub - a lot of the request data doesn't align with the protos (EntityIdentifiers, BoolValue, etc.)
Other than that just small changes! 👍
| return { | ||
| "problems": [ | ||
| { | ||
| "id": res.id, |
There was a problem hiding this comment.
All of the .id (EntityIdentifier) calls in this field will probably need another .id to extract the string. Run the tests forcing RepairTools to v1 to see if it's right
There was a problem hiding this comment.
@jacobrkerstetter Thx! I was not able to run the tests but I will try again with the latest
| This module defines an abstract base class for a gRPC-based repair tools service. | ||
| The class provides a set of abstract methods for identifying and repairing various | ||
| geometry issues, such as split edges, extra edges, duplicate faces etc. |
There was a problem hiding this comment.
we should probably remove this from both v0/v1 files because it is not an abstract base class
|
|
||
| # Create the request - assumes all inputs are valid and of the proper type | ||
| request = FindSplitEdgesRequest( | ||
| bodies_or_faces_ids=kwargs["bodies_or_faces"], |
There was a problem hiding this comment.
Anywhere in this stub that the service wants an id you will need to use build_grpc_id.
There was a problem hiding this comment.
@jacobrkerstetter Sorry about that! Just FYI I started using co-pilot to make this changes but I gave the wrong instructions so it put it on other kwargs as well so I undid it and forgot to put it back properly
| if kwargs["angle"] is not None or kwargs["distance"] is not None: | ||
| # If the backend version is less than 26.1.0, set angle and distance to None | ||
| kwargs["angle"] = None | ||
| kwargs["distance"] = None | ||
|
|
||
| # Log a warning | ||
| LOG.warning( | ||
| "The backend version is less than 26.1.0, so angle and distance parameters will be" | ||
| "ignored. Please update the backend to use these parameters." | ||
| ) |
There was a problem hiding this comment.
It is assumed that v1 users are using 26.1 or newer versions so you can probably take this out
| if kwargs["area"] is not None or kwargs["width"] is not None: | ||
| # If the backend version is less than 26.1.0, set area and width to None | ||
| kwargs["area"] = None | ||
| kwargs["width"] = None | ||
|
|
||
| # Log a warning | ||
| LOG.warning( | ||
| "The backend version is less than 26.1.0, so area and width parameters will be" | ||
| "ignored. Please update the backend to use these parameters." | ||
| ) |
There was a problem hiding this comment.
Same here for the versioning
| # Create the request - assumes all inputs are valid and of the proper type | ||
| request = FindInterferenceRequest( | ||
| bodY_ids=kwargs["bodies"], | ||
| cut_smaller_body=BoolValue(value=kwargs["cut_smaller_body"]), |
There was a problem hiding this comment.
We should have removed these from the protos for v1
| allow_multiple_bodies=BoolValue(value=kwargs["allow_multiple_bodies"]), | ||
| maintain_components=BoolValue(value=kwargs["maintain_components"]), | ||
| check_for_coincidence=BoolValue(value=kwargs["check_for_coincidence"]), |
| from ansys.api.discovery.v1.operations.repair_pb2 import InspectGeometryRequest | ||
|
|
||
| # Create the request - assumes all inputs are valid and of the proper type | ||
| request = InspectGeometryRequest(body_ids=kwargs.get("bodies", [])) |
There was a problem hiding this comment.
Can this match the other methods? ie. kwargs["bodies"] since we control what is passed in here we shouldn't have to have a default empty array
| from ansys.api.discovery.v1.operations.repair_pb2 import RepairGeometryRequest | ||
|
|
||
| # Create the request - assumes all inputs are valid and of the proper type | ||
| request = RepairGeometryRequest(body_ids=kwargs.get("bodies", [])) |
There was a problem hiding this comment.
Same here. And if this is done in v0 we can also change it to align with the normal style
github-actions
bot
added
maintenance
Description
V1 implementation of repair tools. Notes
Issue linked
Please mention the issue number or describe the problem this pull request addresses.
Checklist
feat: extrude circle to cylinder)