Add cq_cache decorator #17

Jojain · 2021-04-01T22:23:29Z

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 !

adam-urbanczyk · 2021-04-02T06:53:07Z

You probably want to use internal data format of OCCT to have no losses. So cq.Shape.exportBrep, I'll add cq.Shape.importBrep to enable this (also needed for CadQuery/cadquery#650 ).

Jojain · 2021-04-02T07:15:25Z

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.

jmwright · 2021-04-02T11:04:44Z

Docmentation link for exportBrep

Jojain · 2021-04-02T11:16:49Z

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

Jojain · 2021-04-08T23:03:19Z

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.
I think I took care of everything but in the end it got a bit more complicated than expected so if you see any flaws let me know.

plugins/cq_cache/README.md

jmwright · 2021-04-13T21:43:04Z

Thanks @Jojain this looks great. I think it would be helpful for there to be docstrings in all the cq_cache.py methods for us to reference in the future, and if I run black against the directory it still shows changes in cq_cache.py. I need to get the lint check fixed for PRs on this repo. I'll try to take a look at that tomorrow.

marcus7070

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

marcus7070 · 2021-04-14T05:01:50Z

plugins/cq_cache/cq_cache.py

+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


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.

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 😄

Yeah, but I'll give you style points for implementing it. 🤣

Co-authored-by: Jeremy Wright <[email protected]>

…into cq_cache

Jojain · 2021-04-18T15:44:58Z

@jmwright @marcus7070 I have added docstrings and tried to use hash instead of str for the identification of passed arguments.
I think the use of hashing for everything would probably better than the use of strings but honestly I don't really understand how it works and I don't want to spend time on trying to figure it out.
If someone more knowledgeable than me on this wants to implement more robust changes I see no problem.
If you are good with the changes we can call it done.

jmwright · 2021-04-18T18:35:19Z

@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.

jmwright · 2021-04-19T13:13:35Z

@marcus7070 I'm ready to merge this. Let me know if you are too.

marcus7070 · 2021-04-19T21:50:57Z

@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

marcus7070

I pushed some changes, have a look @Jojain and let me know if you have any questions or changes.

@jmwright after @Jojain has had a look (and yourself if you want) I'm happy for this to be merged.

Jojain · 2021-04-20T07:17:31Z

plugins/cq_cache/cq_cache.py

+
+ # hash all relevant variables
+ hasher = hashlib.md5()
+ for val in [fct.__name__, args.__repr__(), kwargs.__repr__()]:


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.

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.

Alright thanks for the explanation. But I'm wondering why we can't we pickle CQ object ?

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.

Jojain · 2021-04-20T07:22:28Z

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 !

Jojain · 2021-04-20T08:02:26Z

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)

marcus7070 · 2021-04-20T21:54:18Z

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.

jmwright · 2021-04-21T10:25:43Z

I see that @Jojain gave a thumbs-up on merging, so I'm going to go ahead and do that now. Thanks for the contribution @Jojain !

Jojain and others added 7 commits April 1, 2021 17:17

Create cq_cache.py

f1a40a9

working version of the cq_cache

3bc6e38

Create __init__.py

c666773

creating tests

d9ea7fd

Update test_cq_cache.py

6108068

Update README.md

001f6e1

Update README.md

f5bf632

Jojain added 4 commits April 5, 2021 22:02

add check on the body function

19f74f5

move to BREP files, add good returning type

4e42236

changed from step to brep caching Added logic to handle all the cadquery types and returning the good ones after importing cached geometry

Update test_cq_cache.py

1580b02

Update test_cq_cache.py

25e18c8

added speed gain example to readme

f83e694

jmwright requested review from jmwright and marcus7070 April 13, 2021 18:09

jmwright reviewed Apr 13, 2021

View reviewed changes

plugins/cq_cache/README.md Outdated Show resolved Hide resolved

jmwright reviewed Apr 13, 2021

View reviewed changes

plugins/cq_cache/README.md Outdated Show resolved Hide resolved

jmwright reviewed Apr 13, 2021

View reviewed changes

plugins/cq_cache/README.md Outdated Show resolved Hide resolved

jmwright mentioned this pull request Apr 14, 2021

Equivalent Vscode Extension bernhard-42/jupyter-cadquery#40

Closed

marcus7070 reviewed Apr 14, 2021

View reviewed changes

Jojain and others added 3 commits April 14, 2021 21:46

Update plugins/cq_cache/README.md

4be9336

Co-authored-by: Jeremy Wright <[email protected]>

Update plugins/cq_cache/README.md

3e0f584

Co-authored-by: Jeremy Wright <[email protected]>

Update plugins/cq_cache/README.md

54db9fc

Co-authored-by: Jeremy Wright <[email protected]>

jmwright mentioned this pull request Apr 15, 2021

[WIP] Adding CadQuery support Irev-Dev/cadhub#273

Closed

Add docstrings to functions

1e3d391

Jojain added 5 commits April 18, 2021 16:51

Update cq_cache.py

b627bf6

tried impleting hashed filenames

542b997

removed useless var

41195b5

Merge branch 'cq_cache' of https:/Jojain/cadquery-plugins …

c546e37

…into cq_cache

run black

a8dabd9

jmwright self-requested a review April 18, 2021 18:36

jmwright approved these changes Apr 18, 2021

View reviewed changes

jmwright requested a review from marcus7070 April 18, 2021 18:36

marcus7070 added 3 commits April 19, 2021 17:03

black

68eee00

extra test

ba08ae8

use __name__ and not eval

9ae8f46

marcus7070 added 3 commits April 20, 2021 12:20

Use md5 hash

dfa5f32

* added a test to check if different arguments returned different cache hits * added a test for TypeErrors from Workplane arguments

docs

c33e7f2

better docs

f3ef925

marcus7070 approved these changes Apr 20, 2021

View reviewed changes

Jojain commented Apr 20, 2021

View reviewed changes

jmwright merged commit e62bf3d into CadQuery:main Apr 21, 2021

Jojain deleted the cq_cache branch April 22, 2021 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cq_cache decorator #17

Add cq_cache decorator #17

Jojain commented Apr 1, 2021

adam-urbanczyk commented Apr 2, 2021 •

edited

Loading

Jojain commented Apr 2, 2021

jmwright commented Apr 2, 2021

Jojain commented Apr 2, 2021

Jojain commented Apr 8, 2021

jmwright commented Apr 13, 2021

marcus7070 left a comment

marcus7070 Apr 14, 2021

Jojain Apr 14, 2021 •

edited

Loading

marcus7070 Apr 15, 2021

Jojain commented Apr 18, 2021 •

edited

Loading

jmwright commented Apr 18, 2021

jmwright commented Apr 19, 2021

marcus7070 commented Apr 19, 2021

marcus7070 left a comment

Jojain Apr 20, 2021 •

edited

Loading

marcus7070 Apr 20, 2021

Jojain Apr 20, 2021 •

edited

Loading

marcus7070 Apr 20, 2021 •

edited

Loading

Jojain commented Apr 20, 2021

Jojain commented Apr 20, 2021 •

edited

Loading

marcus7070 commented Apr 20, 2021

jmwright commented Apr 21, 2021

Add cq_cache decorator #17

Add cq_cache decorator #17

Conversation

Jojain commented Apr 1, 2021

adam-urbanczyk commented Apr 2, 2021 • edited Loading

Jojain commented Apr 2, 2021

jmwright commented Apr 2, 2021

Jojain commented Apr 2, 2021

Jojain commented Apr 8, 2021

jmwright commented Apr 13, 2021

marcus7070 left a comment

Choose a reason for hiding this comment

marcus7070 Apr 14, 2021

Choose a reason for hiding this comment

Jojain Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

marcus7070 Apr 15, 2021

Choose a reason for hiding this comment

Jojain commented Apr 18, 2021 • edited Loading

jmwright commented Apr 18, 2021

jmwright commented Apr 19, 2021

marcus7070 commented Apr 19, 2021

marcus7070 left a comment

Choose a reason for hiding this comment

Jojain Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

marcus7070 Apr 20, 2021

Choose a reason for hiding this comment

Jojain Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

marcus7070 Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Jojain commented Apr 20, 2021

Jojain commented Apr 20, 2021 • edited Loading

marcus7070 commented Apr 20, 2021

jmwright commented Apr 21, 2021

adam-urbanczyk commented Apr 2, 2021 •

edited

Loading

Jojain Apr 14, 2021 •

edited

Loading

Jojain commented Apr 18, 2021 •

edited

Loading

Jojain Apr 20, 2021 •

edited

Loading

Jojain Apr 20, 2021 •

edited

Loading

marcus7070 Apr 20, 2021 •

edited

Loading

Jojain commented Apr 20, 2021 •

edited

Loading