Skip to content
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

Fix local ivy import #14262

Merged
merged 4 commits into from
Apr 21, 2023
Merged

Fix local ivy import #14262

merged 4 commits into from
Apr 21, 2023

Conversation

CatB1t
Copy link
Contributor

@CatB1t CatB1t commented Apr 17, 2023

Summary:

  • Add LocalIvyImporter to dynamic_import for local import to work properly.
  • Add local_ivy_id for local ivy id to be used inside impersonate_import to fetch the local object through global ivy.
  • Refactor impersonate_import implementation.

@CatB1t CatB1t marked this pull request as ready for review April 17, 2023 07:41
level=0,
),
)
if local_ivy_id is not None:
Copy link
Contributor

@Infrared1029 Infrared1029 Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following logic can be simplified/unified to look a bit cleaner (the ast.parse is such a nice move tbh makes the code so much more readable), anyway, so we want to have this overall effect:
if local_ivy_id is not None:
we want this to be slapped at the beginning of the file

import ivy
_absolute_import = ivy.utils.backend.handler._compiled_backends_ids[{local_ivy_id}].utils._importlib._absolute_import
_from_import = ivy.utils.backend.handler._compiled_backends_ids[{local_ivy_id}].utils._importlib._from_import

else:
we slap this

from ivy.utils.backend.handler.utils._importlib import _absolute_import
from ivy.utils.backend.handler.utils._importlib import _from_import

we can merge both cases into one

if local_ivy_id is not None:

import ivy
_absolute_import = ivy.utils.backend.handler._compiled_backends_ids[{local_ivy_id}].utils._importlib._absolute_import
_from_import = ivy.utils.backend.handler._compiled_backends_ids[{local_ivy_id}].utils._importlib._from_import

else:

import ivy
_absolute_import = ivy.utils.backend.handler.utils._importlib._absolute_import
_from_import = ivy.utils.backend.handler.utils._importlib._from_import

code-wise it would look something like this:

# ast.parse produces a tree with Module tree, using body[0]
# we can obtain the specific node that we require.
ivy_import_node = ast.parse("import ivy").body[0]
import_stmt = "ivy.utils.backend.handler"
if local_ivy_id is not None:
    import_stmt += f"._compiled_backends_ids[{local_ivy_id}]"
abs_import_stmt = import_stmt + ".utils._importlib._absolute_import"
from_import_stmt = import_stmt + ".utils._importlib._from_import"


abs_import_node = ast.parse(abs_import_stmt).body[0]
from_import_node = ast.parse(from_import_stmt).body[0]
tree.body.insert(self.insert_index, from_import_node)
tree.body.insert(self.insert_index, abs_import_node)
tree.body.insert(self.insert_index, ivy_import_node)

return tree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is definitely shorter, but makes it harder for whoever reads the code to actually know what happens right away, I don't think the tradeoff is worth it, also when debugging, from x import y may be more intuitive than import x; y = x.y

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it is more readable, your point about from x import y being more intuitive is 100% correct, but in this case, we aren't writing code that is explicitly shown, but implicitly injected to do a specific purpose (it is not visible) so I think it is totally fine, a comment (or comments) can be added to explain what is going regardless (whether we go with this way or not actually, a few comments will be needed), I really think this is more concise and easier to debug.

@CatB1t CatB1t merged commit d265bb1 into ivy-llc:master Apr 21, 2023
@CatB1t CatB1t deleted the fix_local_ivy_import branch May 21, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants