Add cq_cache decorator #17
Conversation
|
You probably want to use internal data format of OCCT to have no losses. So |
|
Yes definitely, I searched for this on the cadquery doc but I'm not sure there info on it. I may just have missed it though. |
|
Thanks I'll add this and I may have a solution for returning the right type of objects each time, will do that |
changed from step to brep caching Added logic to handle all the cadquery types and returning the good ones after importing cached geometry
|
This should be done now, I've switched to BREP files ( @adam-urbanczyk I wrote a importBrep function if you want to add it in cadquery) and I added some logic to be able to return the right type of object when decorating a function that returns a cadquery object. |
|
Thanks @Jojain this looks great. I think it would be helpful for there to be docstrings in all the |
There was a problem hiding this comment.
I haven't had a chance to look at this in depth, but this is what I see from a quick look. Very interesting plugin @Jojain!
plugins/cq_cache/cq_cache.py
Outdated
| def using_same_function(fct, file_name): | ||
| with open(file_name,"r") as f : | ||
| cached_function = "".join(f.readlines()[:-1]) | ||
|
|
||
| caching_function = inspect.getsource(fct) | ||
| if cached_function == caching_function: | ||
| return True | ||
| else : | ||
| return False |
There was a problem hiding this comment.
This code on stackexchange might be of interest to you: https://codereview.stackexchange.com/questions/192848/python-function-to-hash-a-python-function
The author does a similar task but he ignores comments, docstrings and minor formatting changes. Also stores the result as a small hash instead of the complete code.
It's a fair bit of extra work though, it's up to you if you want to implement it.
There was a problem hiding this comment.
It doesn't seems too complicated so I might give it a try. Even though I'm not sure it will make more time gain overall than the one it's going to take me to implement this 😄
There was a problem hiding this comment.
Yeah, but I'll give you style points for implementing it. 🤣
Co-authored-by: Jeremy Wright <[email protected]>
Co-authored-by: Jeremy Wright <[email protected]>
Co-authored-by: Jeremy Wright <[email protected]>
|
@jmwright @marcus7070 I have added docstrings and tried to use hash instead of str for the identification of passed arguments. |
|
@Jojain Thanks for all the work you've put into this. The black check here on GitHub always fails for PRs because of a missing feature in GitHub. One thing I have on my list is to run black manually in CI and see if I can get that to work instead of the current Action from the Marketplace. |
|
@marcus7070 I'm ready to merge this. Let me know if you are too. |
Not ready yet, I'm just exploring some of this hashing stuff. Python has deliberately broken the way I want to use the hash function, because repeatable hashes (when used in Python dicts) are a security vunerability. I'll try to push my changes later today. |
* added a test to check if different arguments returned different cache hits * added a test for TypeErrors from Workplane arguments
plugins/cq_cache/cq_cache.py
Outdated
|
|
||
| # hash all relevant variables | ||
| hasher = hashlib.md5() | ||
| for val in [fct.__name__, args.__repr__(), kwargs.__repr__()]: |
There was a problem hiding this comment.
How is this different from my initial way of doing it using str(arg) ?
From what I understand the difference with my initial code is that now instead of storing the str(arg) directly in the file name, you concatene everything is a resulting hash str. What for ?
Oh I wasn't on the right commit but this is still relevant.
There was a problem hiding this comment.
How is this different from my initial way of doing it using
str(arg)?
Yeah, not much difference. I had thought I could get it much more accurate, but the hash implementation is mainly for supporting dicts and isn't suitable.
It's possible to hash the bytes output from pickle. This would be as accurate as I was hoping for, but you can't pickle most CQ types, so it isn't practical for us.
repr is slightly better than str, since:
>>> str(1) == str("1")
True
>>> repr(1) == repr("1")
False
I haven't solved the main problems with a str(arg) based caching though, mainly that any complicated object (including lambdas, generators, cq.Plane and cq.Location) will never compare the same and will be a seperate cache value each time the function is called. 🤷
From what I understand the difference with my initial code is that now instead of storing the
str(arg)directly in the file name, you concatene everything is a resulting hash str. What for ?
The hash string is a fixed length, this could easily blow out past filename limits with large args. Especially given it's going to be used on computationally heavy functions where you wouldn't be surprised to see a large array of (x, y, z) tuples as an argument.
There was a problem hiding this comment.
Alright thanks for the explanation. But I'm wondering why we can't we pickle CQ object ?
There was a problem hiding this comment.
I don't have much experience with pickle. I gave it a try and it errored on simple CQ objects. We might be able to get it working by adding __getstate__ methods on a few CQ classes.
|
I just had some questions but I'm good with the changes ! Thanks @marcus7070 you found a nice way to remove the use of eval. that's great ! |
|
We might also want to wait CadQuery/cadquery#735 to be resolved and use the importBrep implementation from cadquery ? (Altough it should be identical so we can merge it as it is) |
I would say merge now and fix that later. |
Add a file based cache decorator function that allow to cache the result of a function creating a cadquery object.
When exploring design ideas from a complex geometry, rerunning the whole script at each change can be cumbersome. This plugin aims at helping on that.
It does it by writing a step file of the object created by decorated the function the first time a set of parameters is given and import this step if the script/the decorated function is run again with the same parameters.
While writing the tests I realized that the wrapper function doesn't necessarily return the same type of object as the decorated function. That's not goot.
I couldn't come up with a way to check the return object type of the decorated function so I'm asking help on this, if someone as any idea.
A solution could be to force the decorated function to return a specific type of object so the wrapper could return the same, but I think that makes the decorator less useful.
I'm happy to receive your feedback on this, thanks !