feat: add get and create methods and **kwarg support for create#4866
feat: add get and create methods and **kwarg support for create#4866
Conversation
This needs settings api changes that I haven't figured out
There was a problem hiding this comment.
Pull request overview
This PR adds get and create class methods to the object-oriented API for clearer alternatives to using name and new_instance_name parameters. The implementation refactors initialization logic into a shared mixin class and enables passing additional kwargs when creating objects.
Changes:
- Refactored initialization logic into
_SettingsInitializerMixinbase class - Added
getclass method to_SingletonSettingfor retrieving singleton instances - Added
createclass method to_CreatableNamedObjectSettingfor creating new instances with optional attributes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/ansys/fluent/core/solver/settings_builtin_bases.py | Adds mixin class for shared initialization logic and implements get/create class methods |
| src/ansys/fluent/core/solver/flobject.py | Updates _create_child_object and __getitem__ to accept and pass kwargs, enabling attribute setting during object creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Gobot1234 Please can we define the scope of the current work. E.g.:
If we know the scope, it makes it easier to understand the changes. Thanks. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @classmethod | ||
| def get(cls, settings_source: SettingsBase | Solver | None = None, /, *, name: str) -> Self: | ||
| """Get and return the singleton instance of this object in Fluent. |
There was a problem hiding this comment.
The docstring says 'singleton instance' but this method can be used on non-singleton classes like _CreatableNamedObjectSetting. The description should be more generic, such as 'Get and return an instance of this object in Fluent' to accurately reflect its usage across different setting types.
| """Get and return the singleton instance of this object in Fluent. | |
| """Get and return an instance of this object in Fluent. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *, | ||
| name: str, | ||
| ) -> Self: | ||
| """Get and return the singleton instance of this object in Fluent. |
There was a problem hiding this comment.
The docstring describes this method as returning a 'singleton instance', but this is misleading for a named object that can have multiple instances. Consider changing to 'Get and return the named instance of this object in Fluent.'
|
@Gobot1234 In the code snippets in your description, an important use case is binding the solver AxisBoundary(solver).get(name='boundary')
# or hold onto a solver-context-specific AxisBoundary object (which can be passed around)
axis = AxisBoundary(solver)
axis.all()
axis_foo = axis.create(name='foo', other='parameters that', are='just values')
axis_anon = axis.create(other='parameters that', are='just values')This makes the interface more substitutable and the code more flexible, since consumers can Perhaps both formats are already supported in what you have implemented? |
I think I can add support for this form though it'll require more changes. I was honestly thinking the best way to operate on this thing was using the following as it's scoped nicely and closes itself when required with pyfluent.Solver.from_install() as solver, pyfluent.using(solver):
AxisBoundary().get(name='boundary') |
I agree that the context manager pattern is a good one, and it’s something we already encourage for solver lifetime and scoping. However, I think that concern is largely orthogonal to how solver context is modeled in the object API itself. In the pattern you show: with pyfluent.Solver.from_install() as solver, pyfluent.using(solver):
AxisBoundary().get(name='boundary')
Context managers mitigate some ergonomics, but they don’t quite provide the same expressive power as a context-aware object. That’s the reason I prefer constructor binding as the primary model (even if method-level solver passing remains supported). |
@mkundu1 I'd like to get your feedback on this point too. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif k in obj.argument_names: | ||
| newkwds[k] = v | ||
| else: | ||
| elif k not in obj.get_active_child_names(): |
There was a problem hiding this comment.
This condition may not correctly filter out child object parameters passed to create(). If a child name exists but is not 'active', valid kwargs might still be filtered out. Consider validating this logic or adding a comment explaining when child names are 'active'.
| # Generate for all available versions | ||
| versions = sorted([v.number for v in all_versions()]) |
There was a problem hiding this comment.
The comment mentions 'all available versions' but the actual code in if __name__ == '__main__' generates for all versions. This might cause unexpected behavior if run directly vs imported, as the main generation flow uses a single version. Consider clarifying or consolidating the version selection logic.
|
I'm curious as to whether it makes more sense to deprecate the singular or plurals, I think the plural methods make more sense grammatically to get() and all() for but less sense for create() but also interested to hear your opinion @mkundu1 |
| Parameters | ||
| ---------- | ||
| solver | ||
| Something with a ``settings`` attribute. If omitted the active session is assumed from the :func:`using` context manager. |
There was a problem hiding this comment.
Shouldn't use assumed here
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.name = name | ||
| super().__init__( | ||
| settings_source, | ||
| name=name, | ||
| **kwargs, | ||
| _db_name=( # initialise as the parent container | ||
| ( | ||
| DATA[self.__class__._db_name][2] | ||
| if name is None |
| super().__setattr__(name, value) | ||
| def _init_settings_instance( | ||
| self, parent: SettingsBase[object] | ||
| ) -> SettingsBase[object]: |
| root = _get_settings_root( | ||
| solver or _get_active_session() | ||
| ) # validate settings_source | ||
| instance = _CreatableNamedObjectSetting( | ||
| settings_source=root, | ||
| _from_create=True, | ||
| new_instance_name=name, | ||
| _db_name=DATA[cls._db_name][2], | ||
| ) | ||
|
|
||
| obj = _get_settings_obj(root, instance) | ||
| return obj.create(name=name, **kwargs) |
| from typing_extensions import Self | ||
|
|
||
| from ansys.fluent.core.generated.solver.settings_261 import root as Settings | ||
| from ansys.fluent.core.solver.flobject import ( |
| "rename", | ||
| types.MethodType(lambda obj, name: _rename(self, name, cname), ret), | ||
| ) | ||
| ret.set_state(kwargs) |
Up to standards ✅🟢 Issues
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: James Hilton-Balfe <gobot1234yt@gmail.com>
|
CLA Check Failed: Commit Author Verification Unsuccessful One or more commits in this pull request contain missing or invalid author information. This issue may arise due to:
Please update the commit author details and push the updated commit to proceed with the CLA verification. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with open(_PYI_FILE, "w") as f: | ||
| for version in FluentVersion: | ||
| f.write( |
| if settings_source is not None: | ||
| self.settings_source = settings_source | ||
| elif active_session: | ||
| self.settings_source = active_session | ||
| else: | ||
| raise RuntimeError("No active session and no settings_source provided.") | ||
|
|
| super().__init__( | ||
| settings_source, | ||
| name=name, | ||
| **kwargs, | ||
| _db_name=( # initialise as the parent container | ||
| ( | ||
| DATA[self.__class__._db_name][2] | ||
| if name is None | ||
| else kwargs.get("_db_name") | ||
| ), |
| @classmethod | ||
| def all(cls, solver: Solver | None = None, /) -> list[Self]: | ||
| """Return a list of all instances of this object in Fluent.""" | ||
| return cast( | ||
| list[ | ||
| Self | ||
| ], # yes this looks like an unsafe cast but this works via clearing the instance's dict | ||
| list( | ||
| cast( | ||
| Any, | ||
| _NonCreatableNamedObjectSetting( | ||
| settings_source=solver, | ||
| name="*", | ||
| _db_name=DATA[cls._db_name][2], # initialise parent container | ||
| ), | ||
| ), | ||
| ), |
| root = _get_settings_root( | ||
| solver or _get_active_session() | ||
| ) # validate settings_source | ||
| instance = _CreatableNamedObjectSetting( | ||
| settings_source=root, | ||
| _from_create=True, | ||
| new_instance_name=name, | ||
| _db_name=DATA[cls._db_name][2], | ||
| ) | ||
|
|
||
| obj = _get_settings_obj(root, instance) | ||
| return obj.create(name=name, **kwargs) | ||
|
|
| obj = self._objects.get(name) | ||
| if not obj: | ||
| obj = self._create_child_object(name) | ||
| obj = self._create_child_object(name, **kwargs) |
| @@ -0,0 +1 @@ | |||
| Add get and create methods and **kwarg support for create | |||
| return "create" in getattr(cls, "_child_classes", {}) or "create" in getattr( | ||
| cls, "command_names", [] |
There was a problem hiding this comment.
Please can we comment why we check both of these. Otherwise, it looks speculative.
| for legacy_name, v in DATA.items(): | ||
| kind, path, recip = v | ||
| name = _get_public_class_name(legacy_name) | ||
| if isinstance(path, dict): |
There was a problem hiding this comment.
It's not easy to see what the significance is of path being a dict, or what should be done if it is.
There was a problem hiding this comment.
Path here is a dict[FluentVersionSet, str]. It's only used by a small handful of things
{
since(FluentVersion.v251): "setup.dynamic_mesh",
},There was a problem hiding this comment.
Is it expected to be a dict always? IOW is it an error if a different type is encountered there?
There was a problem hiding this comment.
No it's just a dict when there's a version constraint on the path and the name changes between versions.
There was a problem hiding this comment.
OK, please could you add that as a code comment.
| ) | ||
| f.write("\n\n") | ||
| for legacy_name, v in DATA.items(): | ||
| kind, path, recip = v |
There was a problem hiding this comment.
It's not easy to see what recip is.
|
I've had some words with both core (cpython) and ty (the typechecker) devs and I've needed to change the codegen a bit more to change the current settings generator script to output pascal cased names in the pyi file so that things don't break (currently they would if you evaluated them at runtime) and so ty is now better supported. |
The API surface now looks like which is fairly ORM like
Context
Fixes #4863
And adds create and get methods as more clear methods for using name and new_instance_name to the object oriented API
Scope
This PR affects all of the object oriented API, and some of the settings API related code surrounding child creation
This is due to the following changes:
.create()and the object oriented types #4863)