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
Conversation
Just tried it on a Jupyter environment and it works great! ;) maybe I should polish the docstring. 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. |
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? |
Sure. Let me explain the reasoning behind the choice I made for this PR, but definitely your vote weights a lot. We can do 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 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. |
I always use the free functions actually.. >>> import numpy as np
>>> arr = np.array([1,2,3])
>>> arr.sum()
6
>>> np.sum(arr)
6
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 |
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!
I'd totally name you BDFL! 😉 |
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 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 I wouldn't break the tight relationship to the schema on an API that makes things such as 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! ;) |
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!) |
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:
|
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 thegeometry.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 callfile.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 tofile.__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:
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 existingfile.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?