Skip to content

feat: conservative approach to single design per modeler#1740

Merged
RobPasMue merged 7 commits into
mainfrom
feat/conservative-single-design-refactor
Feb 13, 2025
Merged

feat: conservative approach to single design per modeler#1740
RobPasMue merged 7 commits into
mainfrom
feat/conservative-single-design-refactor

Conversation

@RobPasMue
Copy link
Copy Markdown
Member

Description

A single design instance per Modeler object is handled. If a new design is requested, the old design is closed. We are also being conservative and checking that if you use an object belonging to an old design, we are still checking whether the design is active or not. With #1721 we lost this capability. This implementation makes it more robust

Issue linked

Closes #1710

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate unit tests.
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved to the PR if any.
  • I have assigned this PR to myself.
  • I have added the minimum version decorator to any new backend method implemented.
  • I have made sure that the title of my PR follows Conventional commits style (e.g. feat: extrude circle to cylinder)

@RobPasMue RobPasMue self-assigned this Feb 11, 2025
@RobPasMue RobPasMue marked this pull request as ready for review February 11, 2025 14:10
@RobPasMue RobPasMue requested a review from a team as a code owner February 11, 2025 14:10
@github-actions github-actions Bot added the enhancement New features or code improvements label Feb 11, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.80%. Comparing base (dd39086) to head (d74e98d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ansys/geometry/core/modeler.py 93.93% 2 Missing ⚠️
src/ansys/geometry/core/connection/client.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1740      +/-   ##
==========================================
+ Coverage   90.75%   90.80%   +0.04%     
==========================================
  Files          91       91              
  Lines        8007     8003       -4     
==========================================
  Hits         7267     7267              
+ Misses        740      736       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@smereu
Copy link
Copy Markdown
Contributor

smereu commented Feb 11, 2025

@RobPasMue I played around with a few things with Discovery and core services and I didn't encounter any issue. Looks good!

I have a few comments that are minor and can probably be addressed (if needed) after the PR is merged

  1. I see that modeler.designs[] is marked as obsolete but is there way to know that it is the case? I only know because I looked at the code :-)
  2. When you close a design, its property is_active becomes False. There is also a property is_alive (as it derives from Component) that is still set to True. Maybe it will disappear with the next round of refactoring. If not, we can set it to False as well
  3. I see that there is a call to _activate(). Hopefully no code path will hit it, but I wonder if it can/should be removed
  4. The decorator @ensure_design_is_active is used in many places. As we do the next round of refactoring maybe we can do something like that internally (since we won't cache anything) so we remove the need to have it almost everywhere

@RobPasMue
Copy link
Copy Markdown
Member Author

RobPasMue commented Feb 12, 2025

  1. I see that modeler.designs[] is marked as obsolete but is there way to know that it is the case? I only know because I looked at the code :-)

The user will get a warning in their logs whenever they perform a call to designs. It's up to the user to read the logs and see deprecated methods from there onwards :)

  1. When you close a design, its property is_active becomes False. There is also a property is_alive (as it derives from Component) that is still set to True. Maybe it will disappear with the next round of refactoring. If not, we can set it to False as well

Hmm the is_alive boolean was used for different purposes (i.e. to determine if a body/component has been deleted or not). All the modeling operations should have the @ensure_design_is_active decorator so we should be safe there. If a user still wants to query an old component for info such as... it's name? They could still do it. And I agree that maybe it will just get removed in the next refactor. If it doesn't we can decide then.

  1. I see that there is a call to _activate(). Hopefully no code path will hit it, but I wonder if it can/should be removed

There are 2 calls to it in fact. But the method is still needed so that the client side still knows that a design is active.

  1. The decorator @ensure_design_is_active is used in many places. As we do the next round of refactoring maybe we can do something like that internally (since we won't cache anything) so we remove the need to have it almost everywhere

That decorator is still needed IMO as long as the caching still exists. But I don't envision it disappearing. It depends on the depth of the caching we will do honestly. We can discuss this later. But even if we remove caching... whenever a user "stores" an object, we have to be conservative and assume that the user could try to use it later... so we will still need the decorator to tell them that the design is closed and this object can no longer be used.

Comment thread src/ansys/geometry/core/modeler.py
@RobPasMue RobPasMue merged commit e91ffe2 into main Feb 13, 2025
@RobPasMue RobPasMue deleted the feat/conservative-single-design-refactor branch February 13, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New features or code improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Single design instance per Modeler object

4 participants