fix: deepcopy fail for coping model_instance config #613
Conversation
goasleep
changed the title
goasleep
force-pushed
the
feature/add_copy_tool
branch
2 times, most recently
from
deba404 to
d1926a4
Compare
|
In addition, there are any formattor about this project? seem I cannot find it |
scrapegraphai/utils/copy.py
Outdated
| from typing import Any, Dict, Optional | ||
|
|
||
|
|
||
| def safe_deepcopy(obj: Any, memo: Optional[Dict[int, Any]] = None) -> Any: |
There was a problem hiding this comment.
why memo and not a more standard cache?
There was a problem hiding this comment.
I deleted it because I found it not very necessary and may cause other problems. BTW,memo is the second parameter name of deepcopy so I use it.
scrapegraphai/utils/copy.py
Outdated
| pass | ||
|
|
||
| # If all else fails, return the original object | ||
| return obj |
There was a problem hiding this comment.
defaulting to returning the original object might be a bad idea for a number of reasons, if misused (e.g., assuming on the outside the copy always works)
would either remove it, throwing an error if even normal copying doesn't go through, or adding a parameter strict or something regulating the behavior
There was a problem hiding this comment.
I change it and raise custom error DeepCopyError. and I also add pydantic object copy deep function because I found out that he didn’t support it before.
goasleep
force-pushed
the
feature/add_copy_tool
branch
from
d1926a4 to
36818b1
Compare
|
wait a moment. find new problem |
|
What's the problem? |
I found there is message missing when use
|
|
Now is ok? |
yeah. Now is ok. |
|
🎉 This issue has been resolved in version 1.16.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
thank you! |
|
🎉 This issue has been resolved in version 1.16.0-beta.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
link #609
When copying the configuration with model_instance, an error may arise because model_instance is typically an uncopyable object. To fix it, I attempted to use safe_deepcopy to replace the internal deepcopy during the copying of the LLM configuration.
The safe_deepcopy function first attempts to use the internal deepcopy, ensuring it doesn't disrupt the original usage. If that fails, it resorts to alternative copying logic, such as shallow copying. In the worst-case scenario, it returns the original object.
Note:
If there are any new graphs using deepcopy in future pull requests, special attention should be paid to copying the LLM configuration correctly.
Other Considerations:
This pull request may not represent the best practice for resolving the issue, but it offers a quick fix. Here are some alternative approaches that I believe are superior: