Skip to content

FIX: Connect to existing student version - #7918

Open
Samuelopez-ansys wants to merge 17 commits into
mainfrom
fix/issue_7891
Open

FIX: Connect to existing student version#7918
Samuelopez-ansys wants to merge 17 commits into
mainfrom
fix/issue_7891

Conversation

@Samuelopez-ansys

@Samuelopez-ansys Samuelopez-ansys commented Jul 15, 2026

Copy link
Copy Markdown
Member

Description

Student version was connecting to existing session.

Platform: windows, linux

Issue linked

Close #7891
Close #7884

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 tests (unit, integration, system).
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue(s) that are solved by 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 agreed with the Contributor License Agreement (CLA).

@Samuelopez-ansys
Samuelopez-ansys requested a review from a team as a code owner July 15, 2026 08:26
@Samuelopez-ansys Samuelopez-ansys changed the title Fix/issue 7891 FIX: Connect to existing student version Jul 15, 2026
@github-actions github-actions Bot added bug Something isn't working windows Windows-specific issue or change linux Linux-specific issue or change labels Jul 15, 2026
maxcapodi78
maxcapodi78 previously approved these changes Jul 15, 2026
eblanco-ansys
eblanco-ansys previously approved these changes Jul 15, 2026
@Samuelopez-ansys
Samuelopez-ansys enabled auto-merge (squash) July 15, 2026 09:25
SMoraisAnsys
SMoraisAnsys previously approved these changes Jul 15, 2026
@Samuelopez-ansys
Samuelopez-ansys marked this pull request as draft July 15, 2026 10:40
auto-merge was automatically disabled July 15, 2026 10:40

Pull request was converted to draft

@Samuelopez-ansys
Samuelopez-ansys marked this pull request as ready for review July 16, 2026 11:58
@tzhou-yyds

Copy link
Copy Markdown

Verified on Windows 11, AEDT 2026 R1, this PR branch (1.3.dev0):

Attach to existing session works — started ansysedt.exe -grpcsrv 50700 -ng manually, then Desktop(version="2026.1", port=50700, new_desktop=False, non_graphical=True) connects to the same process (verified odesktop.GetProcessID() == manual instance PID, ruling out a silent new instance). release_desktop(close_projects=False, close_on_exit=False) leaves the instance alive as expected. This resolves our #7884 local-attach scenario.

⚠️ Two observations from the same test bench:

  1. With the legacy workaround env var PYAEDT_USE_PRE_GRPC_ARGS=True set, the same attach silently launches a new instance instead (PID mismatch, no error raised). Users who adopted that workaround for Attaching to a remote gRPC session validates the port on localhost: _validate_port() called without machine #7884 will hit silent failures after upgrading — a code warning or docs note might be worth it.
  2. After an abnormally interrupted session, re-attaching to the same port can also silently spawn a new instance until stale processes exit.

Remote-machine attach (the original #7884 remote case) was not tested — no routable gRPC path between our two machines.

@Samuelopez-ansys

Copy link
Copy Markdown
Member Author

Hi @tzhou-yyds , thanks for the feedback, I applied a small fix that could impact the issue you mentioned, I tested 2026R1:

app_1 = Hfss(version="2026.1", port=50700, new_desktop=False, non_graphical=True, machine="127.0.0.1")

With an open AEDT in non graphical mode with the env. variable PYAEDT_USE_PRE_GRPC_ARGS, and it not open a new AEDT instance.

@tzhou-yyds

Copy link
Copy Markdown

Re-tested with the latest fix/issue_7891 (41658c8) on 2026R1, Windows 11 — the small fix works:

  • Case B (PYAEDT_USE_PRE_GRPC_ARGS=True + Desktop(version='2026.1', port=50700, new_desktop=False, non_graphical=True)): attaches to the manually launched ansysedt -grpcsrv 50700 -ng instance, GetProcessID() matches the manual PID — no new instance spawned (previously this env var caused a silent new instance).
  • Case C (your exact form: Hfss(version='2026.1', port=50700, new_desktop=False, non_graphical=True, machine='127.0.0.1') with the env var set): attach OK, PID matches as well.
  • release_desktop(close_projects=False, close_desktop=False) leaves the manual instance alive as expected in both cases.

Thanks for the quick turnaround!

@SMoraisAnsys SMoraisAnsys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall LGTM but there is something bothering me with "Step 6". Otherwise, I left minor comments.

for el in all_sessions.values():
if self.port in el.values():
self.logger.warning(f"Port {self.port} is already in use. Finding a new free port.")
self.port = _find_free_port()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we shoudl directly play with __port instead of the port property. That way we are sure that any change in the property (like this PR) wouldn't affect the inner logic. Open to discussion though :)

Comment on lines +1599 to +1609
# Step 6: Fallback method - Try to find ports by checking TCP network connections
for version, sessions in return_dict_filtered.items():
if any(port == -1 for port in sessions.values()):
for pid in [i for i, v in sessions.items() if v == -1]:
version_number = version.replace("_student", "").replace("_nongraphical", "").replace("_graphical", "")
sessions[pid] = _check_connection_grpc_port(
connections,
pid,
version_number,
True if "nongraphical" in version else False,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how this step interact with the returned value, isn't there something missing here ?



@pyaedt_function_handler()
def all_active_sessions() -> dict[str, dict]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def all_active_sessions() -> dict[str, dict]:
def all_active_sessions() -> dict[str, dict[int, int]]:


Returns
-------
dict[str, dict]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dict[str, dict]
dict[str, dict[int, int]]

Returns
-------
dict[str, dict]
Dictionary mapping AEDT process IDs to their corresponding ports.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Dictionary mapping AEDT process IDs to their corresponding ports.
Dictionary mapping AEDT version to the associated process IDs and their corresponding ports.

@pyaedt_function_handler()
def _assign_port(self):
self.__port = 0
self.port = 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same remark as above, I would keep the __port value



"""
# Step 1: Determine target process names based on version type and operating system

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Steps do not align with the docstring description and misses the second step :p

assert d2.new_desktop


def test_new_session_port_0(mock_settings):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please split that into multiple tests isntead of one gathering everything. Same comment for the following ones

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

Labels

bug Something isn't working linux Linux-specific issue or change windows Windows-specific issue or change

Projects

None yet

6 participants