-
Notifications
You must be signed in to change notification settings - Fork 85
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
remove from baseutils import *
#1421
Conversation
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
I never thought `Optional` is imported from `baseutils`, this does not feel great to me. Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
b6a9281
to
b987ee8
Compare
@@ -19,7 +20,6 @@ | |||
import thunder.core.dtypes as dtypes | |||
import thunder.core.devices as devices | |||
from thunder.core.pytree import tree_flatten, tree_unflatten | |||
from thunder.core.baseutils import * |
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.
It's OK to replace uses of baseutils
with explicit baseutils.foo
calls in this file, but conceptually baseutils
, codeutils
and utils
offer a progressive expansion of utility functions. Most files can just import utils
(and doing so gives them all the functions in codeutils
and baseutils
), but some files can only import codeutils
or baseutils
. baseutils
depends only on Python and PyTorch. codeutils
depends on thunder's dtypes
, devices
and pytree
submodules. utils
also depends on prims
, trace
and proxies
.
For files that cannot import utils
but can import codeutils
, they probably want baseutils
, too, just like how importing utils
provides access to the functions in baseutils
and codeutils
.
There are some files, like trace.py
, that import both codeutils
and baseutils
, but they could just import codeutils
, instead.
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.
Let's not aim to minimize the number of import lines but maximize that there is one standard way to access a given function and only deviate from it when there is a need to. I don't mind if we use baseutils.foo or utils.foo by default but let's not do a thing codeutils.foo if you import codeutils and otherwise baseutils.foo .
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.
Thank you @crcrpar @IvanYashchuk @mruberry
What does this PR do?
Stop start import in codeutils.