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

Patch API calls to ifcopenshell.api #3830

Closed
wants to merge 6 commits into from

Conversation

cvillagrasa
Copy link
Member

This PR is related to #2693. It adds static type decorations without changing the underlying architecture of the Usecases and the centralized run. Or, more specifically, it grabs the signatures from the Usecases, so to take full advantage of the benefits of static typing in smart IDEs or Jupyter Notebooks, they'd also need to update its signature, as proposed in #3774 for the geometry.add_wall_representation case.

The proposal consists of the simple addition of patch_api method to IfcOpenShell files. For now, nothing happens if one doesn't explicitly call file.patch_api(), so it is safe to merge in case this strategy sounds good. If/when it is fully agreed, it can then be included to file.__init__ and patch all new files by default at runtime. I guess it could also be added to the cmake in order to patch the file class at install time, by making it a classmethod, but I can't think of a straightforward way to reach the file instance in that case.

It is worth noting that there aren't as much API calls so as to add a very significant overhead. There's a glob over there to find the Usecase files, but a call to patch_api takes 100 ms on my system. I'd say it's acceptable.

With this design, one calls the API following the dot notation from a file:
patch_api

I find it very user friendly, even better than what I was proposing before of ios.root.create_entity(file, "IfcWall"). It more or less follows the notation of the existing file.create_entity, and the dot notation, grouping by API module, makes the namespace not super polluted, at least in my view.

What do you think of an approach like this, @aothms @Moult and everyone interested?

@cvillagrasa
Copy link
Member Author

Just tried it on a Jupyter environment and it works great! ;) maybe I should polish the docstring.

patch_api_01

patch_api_02

BTW, for this PR I was focused on Jupyter, now re-reading my message above... including also IDEs wasn't correct. So this PR only brings the benefits of static typing to Jupyter, with a live session, unfortunately not to IDEs.

@aothms
Copy link
Member

aothms commented Oct 1, 2023

I'm fully inline with the goals, but I'm not sure if putting this on the file instance makes most sense. What if we later have API calls with multiple files (which I guess is already somewhat possible with templates and all).

Can't you monkey patch the ifcopenshell.api module itself?

@cvillagrasa
Copy link
Member Author

Can't you monkey patch the ifcopenshell.api module itself?

Sure. Let me explain the reasoning behind the choice I made for this PR, but definitely your vote weights a lot. We can do import ifcopenshell.api as ios and then ios.root.create_entity(file, "IfcWall"). Do you like ios as an alias?

The reasoning behind: It was to follow the user-friendliness of numpy, pandas or any of those Data Science libs, which provide a ton of methods on instances. As for API calls on multiple file instances (are there any, yet? what's possible with templates?), they look fundamentally different to me, so no question that they should be placed under ifcopenshell.api. The same way that np.vstack((arr1, arr2)) needs to come from numpy and my_array.flatten() can be called as a method of the ndarray instance. BTW, what happens with file.create_entity? should it be homogeneized with the rest of the API?

I kind of like the semantics of "On this file, I'm performing this action, with these arguments" rather than "I'm performing this API action with these arguments, being the file one of them". But of course this is a very minor nuance, and the most important bit is to have hinting aids.

@aothms
Copy link
Member

aothms commented Oct 2, 2023

It was to follow the user-friendliness of numpy, pandas or any of those Data Science libs, which provide a ton of methods on instances

I always use the free functions actually..

>>> import numpy as np
>>> arr = np.array([1,2,3])
>>> arr.sum()
6
>>> np.sum(arr)
6

Do you like ios as an alias?

Not a fan, but I'm not a dictator either. I would probably write:

from ifcopenshell import api as ifc
ifc.create_entity()

let's see what the other say

@cvillagrasa cvillagrasa changed the title Patch API calls as methods of an ifcopenshell.file Patch API calls to ifcopenshell.api Oct 2, 2023
@cvillagrasa
Copy link
Member Author

I always use the free functions actually..

Yeah, I guess it's a matter of preference. I moved it to ifcopenshell.api, with the name ifc. Note that patching it to api directly would collide with the actual Usecase modules. So now, the current state of this PR allows for the following:

from ifcopenshell.api import ifc

file = ifc.project.create_file()
wall = ifc.root.create_entity(file, ifc_class="IfcWall")

My first thought for the alias also was ifc, then I thought it might get confused with the file (since some people call the file instances ifc), but the truth is even the file shouldn't be called file. Dion often calls them model, which in turn can collide with the model context... and it never ends. Choosing ios could help avoid some of these collisions, but maybe it's true the naming can feel foreign. So happy with ifc!

but I'm not a dictator either

I'd totally name you BDFL! 😉

@cvillagrasa
Copy link
Member Author

Following up with #3774 and this PR, let me guys restate a bit the spirit of this set of PRs regarding ifcopenshell-python.

If in IFC, we have different releases / addendums / corrigendums, I believe that it should be possible to initialize a model of any schema definition, and yet have type hinting benefits according to that specific definition. Or what's the same, I find it very annoying as a user not to have those benefits, that I would expect in every modern Python library.

The classical example I use to illustrate this is material.add_layer. If we could pass it every IFC layer related arg for it to be correctly constituted, we could for instance use the kwarg category="Insulation", so long as the version is >= IFC4. So category should appear in the signature of an IFC4 model, but not if it was IFC2x3. And here's the thing, not having this distinction (as it is now) makes it easy to develop, but the user has to then monkey-patch schema attributes with the corresponding bSI docs opened in the backgroung. But... having this distinction without any Python trick requires to have an ifcopenshell-python distribution per schema definition.

To be clear beyond this PR, I believe the way to go is precisely to have one ifcopenshell-python distribution per schema definition. Not by maintaing all of them, for sure, but by autogenerating schema-dependent modules. Then, there would be no need to ship them all together, and we could manage optional dependencies and do perhaps pip install ifcopenshell[IFC4] or pip install ifcopenshell[IFC4X3] or explicitly pip install ifcopenshell[all], with maybe pip install ifcopenshell getting IFC4 and IFC4x3. Am I dreaming?

I wouldn't break the tight relationship to the schema on an API that makes things such as add_layer, referring to an IfcMaterialLayer. I'd leave that to higher level stuff built on top with something like add_fancy_floor.

And to come back to this PR (and #3774 and #3851), I've found these techniques to be a pragmatical way of having type hinting benefits in the short term, specially in notebook environments. So my idea was to experiment with a friendly API first, to then manufacture some CI/CD to produce the multischema API.

And that's my take! ;)

@Moult
Copy link
Contributor

Moult commented Nov 15, 2023

we could for instance use the kwarg category="Insulation", so long as the version is >= IFC4. So category should appear in the signature of an IFC4 model, but not if it was IFC2x3.

What if the category is always there as an arg, but silently ignored if it's IFC2X3?

I see two parts of this PR. The first is the difference between passing API calls as a string to run() which is totally undiscoverable with no autocompletion, no docstring fetching, and no signature previews in IDEs and interpreters. For this, 100% agree needs to be fixed ASAP.

The second aspect is the actual signature design. How much type hinting is done, how strict should they be at runtime, how much validation needs to be done, how they handle schema differences #3851 (manual definition? autogenerated at runtime? autogenerated at buildtime?). Note that even though BBIM has no support for Python <3.10, IOS is still distributed down to Python 3.6 so we need to make a call. I believe type hinting is 100% a good idea. Runtime checking and schema differences I'm uncertain how to handle. I'll answer more on #3851 and #2878. (wow, this is turning into a maze of issues and PRs, really gotta get some closure on this!)

@Moult
Copy link
Contributor

Moult commented May 7, 2024

I'm closing this PR because after a lot of discussion and consideration, I don't think putting the API on the file object is a good idea. Reasons:

  • Currently, API functions expects a single file. In the future, it may not.
  • The API is big, and will grow in the future. I don't want to enforce it to be loaded. It should be possible to work with the core ifcopenshell at a low level with no api/utils.
  • Keeping the file lean also makes it easier to support wasm.
  • Patching it at runtime doesn't work for static analysis and in most IDEs. Generating code to support it on the file means work to support two ways to do the same thing with different syntactic sugar. Refactoring code statically to only work on the file enforces large dependencies the moment someone creates a file object.

@Moult Moult closed this May 7, 2024
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

4 participants