Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for passing IFC Schema attributes on API methods #3851

Open
wants to merge 23 commits into
base: v0.7.0
Choose a base branch
from

Conversation

cvillagrasa
Copy link
Member

This PR includes a with_schema_attrs decorator that allows for the following (material.add_layer example):

@with_schema_attrs(ifc_class="IfcMaterialLayer", exclude=["Material"], defaults={"LayerThickness": 1.})
@dataclass
class Usecase:
    file: ifcopenshell.file
    layer_set: ifcopenshell.entity_instance
    material: Optional[ifcopenshell.entity_instance] = None

    def execute(self):
        layers = list(self.layer_set.MaterialLayers or [])
        layer = self.file.create_entity("IfcMaterialLayer", Material=self.material, **self.schema_attrs())
        layers.append(layer)
        self.layer_set.MaterialLayers = layers
        return layer

Here, attributes from IfcMaterialLayer will be parsed from the schema (with a version matching file), except Material (since it's already the snake case material argument due to its importance), and with a default value of 1.0 for LayerThickness. Then, a self.schema_attrs() method is available to retrieve a dict containing only those schema attributes from within the class body.

The usecase help then looks as follows (with API patching from #3830). Note that annotations work with aggregations, select types, etc. I used the available methods on schema declarations for that, recursively. Maybe there was some better way, curious to learn about it if there was!

bitmap

And it works as expected, if no LayerThickness is passed, 1.0 will be used. If a Description is used, it will be baked into the IfcMaterialLayer, etc.

mat_layer

Thoughts on this one? it's related to #2878. I find that when an API call is so closely related to the schema, it makes sense to also have the official pascal case attributes available.

As #3830, it relies on signatures and would need to evolve into a more static approach in the future. However, in this case, there are several schemas... I can only think of making file subclasses for each schema, and cmake-generate the corresponding API methods for each one, in the fashion @aothms doesn't like (ifc4file.material.add_layer...). Not the prettiest solution, I know, I'll keep thinking.

Also, the pipe operator generating types.UnionType is available from Python 3.10. Is it really needed to support 3.6 to 3.9 for ifcopenshell.api?

@Moult
Copy link
Contributor

Moult commented Nov 15, 2023

This is super cool, great idea to parse the schema declaration to decorate function signatures. But does this actually work in an IDE (not Jupyter) and does it detect it properly? I noticed some magic that listens for module importing and patches it then.

@cvillagrasa
Copy link
Member Author

@aothms @Moult @Andrej730

This is the PR that started as a live signature patching, and evolved towards a build step using AST after firstly discussing it with Dion and Andrej. Even if we've talked about it during our meeting, let me write down some background for reference, mixed with some sort of MoM.

Background

The approach is conceived in an environment in which the Python API calls were fully type hinted. By bypassing the run centralized call, it adds these type hinting back to methods appended on the file object, so that the user maybe knows that the material argument is an ifcopenshell.entity_instance instead of a str with the material name, by starting to type file.material.add_profile(mat.... Up to this point, nothing schema-dependent yet.

In my head, it made sense to extend this hinting to API calls closely tied to the schema, such as the well-known material.add_layer, that adds an IfcMaterialLayer, which would be nice that accepted Category, IsVentilated, so that there's no need to monkey-patch or to make a call to material.edit_layer with a dictionary which requires documentation. And again in my head, the fact that API calls named add_context (btw this is the one to blame for getting me started with this particular fight 😅), add_layer, or add_material_layer exist, indicates that at least part of the API is still ultra-close to the IFC schema. So the question arises, does it make sense to have schema version-oriented APIs? The answer to which as you know, yet again in my head, was yes.

And if the answer is yes, it starts to make more sense to append API methods to the file object, rather than having different APIs and having to choose. And this brings us to the status of this PR as of now.

Balance

So, just to list some of the benefits, as discussed during the meeting:

  • It adds "strong" type hinting (working both in REPLs and IDEs, unlike signature patching). We all agreed on this one.
  • It makes the API more user-friendly, rather than "it's just going to be used by devs, they'll research". So more like numpy, less like matplotlib and its keyword arguments if that makes sense (even though matplotlib is ultra popular and works well, so this point can perfectly not matter too much depending on your preference, as it is the case for Thomas and Dion). My particular take is that if the API works superwell with linters, it feels less experimental and that 10% of the 10% of the 10% of AEC professionals Dion often refers to, might produce higher quality downstream libs, which can in turn benefit us all. It also makes it much easier for beginners using Jupyter Notebooks to learn the library.
  • It standardizes things and provides an automated way to digest new schema versions like Next Gen (even though the important bit, the actual API calls, may need to be adapted to new/deprecated features, of course).
  • It works either with Usecase.__init__ signatures, or a dataclass Usecase.

And some drawbacks:

  • It is costly. It adds more weight to the codebase. To minimize it:
    • Identic calls accross schema versions could be smartly reused.
    • Docstrings could be passed only in one place (api or file appended methods, but not necessarily both).
    • Different distributions could be made without the type hinting overhead (for instance for wasm), or to someone knowing for sure they'll only use IFC4, so maybe optional dependencies could be distributed, like pip install ifcopenshell[IFC4] or pip install ifcopenshell[all].
    • If we try to bring in Some mypy experiments #3306 to this approach, and type hint beyond ifcopenshell.entity_instance, to maybe ifcopenshell.entity_instance[IFC4.IfcWall], the weight of the codebase increases even more (and so does the user friendliness).

Some further points

In this PR, there is one centralized run method for all APIs, so one set of pre-listeners and post-listeners. This makes sense to me, but when looking at the project structure it looks odd. And even odder would look to have several runs. Rather than having one class named Usecase on each API action, maybe I'd suggest to define a general Usecase base class, with class methods handling the listeners as a singleton, and then, each action class could inherit from it (as in AddLayer(Usecase)). Then, auto-generated snake-case methods could be appended to the file object as done in this PR.

Or also a more functional approach with a traditional snake-case function, with decorators handling the listeners, as discussed in #2693 .

Any of the aforementioned two approaches would mean equivalence between calling API actions from file-appended methods and from ifcopenshell.api_<version>, which would of course be desirable. As it is now, though, with the listener handling being done in the run method, it only works from file-appended methods (which in turn call run). Calling directly ifcopenshell.api_<version> will skip this.

Also, the integration with the file class is done pretty quickly and should be rethought.

Future steps

I know I'm mixing very fundamental topics as from #2693 and #3306 , altogether with an AST-based build step, but I feel all of these are closely related, and choosing one approach in any of this isolated issues does affect the options for the others.

I'll try to think of the reusing schema-built code across versions. Maybe it's as "easy" as implementing an __equal__ for SchemaApiActionBuilder and memoizing them, although it would quickly get complicated if we wanted to go for the optional dependency installation.

Now, with all of it written down, I'm not sure any of this is going to make it through the scheduled release of the 6th of January, @Moult , maybe it's better to attempt for the next.

@Moult
Copy link
Contributor

Moult commented Nov 28, 2023

Or also a more functional approach with a traditional snake-case function, with decorators handling the listeners, as discussed in #2693 .

I would prefer this approach. It seems simpler and more elegant than adding more classes. Here's a slight variation on @aothms' original suggestion in #2693 that doesn't rely on duplicating the signature.

# in api/root/__init__.py

import ifcopenshell.api.root.create_entity as create entity

# in api/root/create_entity.py

@my_decorator_that_injects_run_listeners
@my_decorator_that_hints_about_schema_differences_or_schema_signature_stuff
def create_entity(file, ifc_class="...", ...):
    """The docstring of the create entity stuff..."""
    usecase = Usecase()
    usecase.settings = { ... }
    return usecase.execute()

... in other words, just take the __init__ function of the Usecase out into a top level function and use decorators. The module's __init__.py file can then import those functions.

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

Successfully merging this pull request may close these issues.

None yet

3 participants