-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Update: compile updated to trace in ivy repo #22854
Update: compile updated to trace in ivy repo #22854
Conversation
Thanks for contributing to Ivy! 😊👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume a lot of tests are failing due to what I pointed out, though I can't seem to confirm this with the CI (it just seems to show a long list of what's failing without the error messages).
Also I think you missed this https://github.com/unifyai/ivy/blob/main/ivy/__init__.py#L791 - presume this try
will fail also when trying to import compile
which is now trace
Yeah I missed that 😅. Thanks for pointing out! |
heads up before leaving a review, all the module stuff is being refactored at the moment afaik, so it will most likely become a nightmare to merge the changes 😅 I'd say better to wait to change this if the module refactor is going to be merged soon (which I assume it is) |
Yeah, I'll wait for it @guillesanbri 😄. |
ivy/compiler/compiler.py
Outdated
from ._compiler_38 import compile as _compile | ||
from ._compiler_38 import trace as _trace | ||
else: | ||
from ._compiler import compile as _compile | ||
from ._compiler import trace as _trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small heads up: this will need to be changed to from ._compiler import compile as _trace
as the .so won't be updated yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep!
…nArc/ivy into compile_to_trace_ivy
cb90313
to
cc7e2bc
Compare
ivy/__init__.py
Outdated
@@ -812,7 +812,7 @@ class Node(str): | |||
except: | |||
pass | |||
try: | |||
from .compiler.compiler import transpile, compile, unify | |||
from .compiler.compiler import transpile, trace, unify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trace_graph
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I searched for trace(
LOL. Obviously it would get missed. Sorry for that 😅
ivy/utils/_importlib.py
Outdated
spec.loader.exec_module(module, ivy._compiled_id) | ||
spec.loader.exec_module( | ||
module, ivy._compiled_id | ||
) # ! Not sure about what this ID is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is related we can remove the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
ivy/utils/exceptions.py
Outdated
@@ -28,22 +28,22 @@ def _remove_so_log(trace): | |||
if ".pyx" in repr(st): | |||
continue | |||
if "<string>" in repr(st): | |||
if "compiled_fn" in repr(st) and module_frame: | |||
if "traced_fn" in repr(st) and module_frame: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably needs to be compiled_fn
until the changes are effective in the .so file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense!
ivy/utils/exceptions.py
Outdated
|
||
if "<module>" in repr(st): | ||
module_frame = old_frames[idx] | ||
module_st = st | ||
elif ( | ||
transpile_frame is None | ||
and os.path.join("ivy", "compiler") in st.filename | ||
and st.name in ["compile", "transpile"] | ||
and st.name in ["trace", "transpile"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not entirely sure about this one, so just highlighting in case it also needs to be updated only once the .so file is updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's comming from .so
file so it would be compile for now
We are updating our
ivy.compile( )
function toivy.trace( )
function.This requires updating the Docs, Ivy Repo, Graph Compiler Repo, Demos, and other places to refer to Tracer rather than Graph Compiler!
This PR updates Ivy Repo Code!