ansys / pyfluent

Pythonic interface to Ansys Fluent
https://fluent.docs.pyansys.com
MIT License
282 stars 41 forks source link

FluentConnection class refactoring #2609

Closed seanpearsonuk closed 3 weeks ago

seanpearsonuk commented 8 months ago

FluentConnectionProperties encapsulates the business data for FluentConnection. FluentConnection should not have other dependencies.

Glancing at its constructor, launcher_args stands out as a false dependency of FluentConnection. The constructor uses it to obtain any SLURM job ID: self._slurm_job_id = launcher_args and launcher_args.get("slurm_job_id"). An optional slurm_job_id is a better (imperfect) construction dependency. Note that self._slurm_job_id is used only at exit. This implies that we can later layer things to keep such details out of the raw connection class.

FluentConnection::__init__ creates a SchemeEvalService. This is too concrete. What is it used for?

            fluent_host_pid = self.scheme_eval.scheme_eval("(cx-client-id)")
            cortex_host = self.scheme_eval.scheme_eval("(cx-cortex-host)")
            cortex_pid = self.scheme_eval.scheme_eval("(cx-cortex-id)")
            cortex_pwd = self.scheme_eval.scheme_eval("(cortex-pwd)")
        build_time = self.scheme_eval.scheme_eval("(inquire-build-time)")
        build_id = self.scheme_eval.scheme_eval("(inquire-build-id)")
        rev = self.scheme_eval.scheme_eval("(inquire-src-vcs-id)")
        branch = self.scheme_eval.scheme_eval("(inquire-src-vcs-branch)")
        scheme_eval.exec(("(exit-server)",))

We should design Python interfaces for these. In the first instance, we can implement them in Python, wrapping SchemeEval. Later, we can move the implementation into the server behind gRPC. The first two chunks can all go behind a ServerInfo interface. The last can be in a ServerControl interface.

seanpearsonuk commented 3 weeks ago

@prmukherj Noting that I reopened this in April but since no reason was given, I will assume that there is nothing more to do until we see something concrete in future.