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 duplicated imports #1133

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Apr 9, 2020

What this PR does / why we need it:

  • Add __init__.py to avoid using namespace package.
  • Fix duplicated import problems by __all__ attribute.
    • To clarify this problem, I attached a screenshot of PyCharm.
    • ScreenShot 2020-04-09 23 15 58
    • logging is imported at L3 and used at L8. But Jetbrains IDE highlights import logging as gray. This means it is an unused line. The reason why it is highlighted as gray is using star imports ( from pkg.suggestion.v1alpha3.internal.search_space import * and from pkg.suggestion.v1alpha3.internal.trial import * ). To avoid this problem, I define __all__ attribute. See https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
None

Special notes for your reviewer:

None

Release note:

NONE

@kubeflow-bot
Copy link

This change is Reviewable

@c-bata c-bata force-pushed the fix-duplicated-imports branch from f99736d to 762e6a0 Compare April 9, 2020 16:01
@c-bata c-bata force-pushed the fix-duplicated-imports branch from 762e6a0 to f2c5f37 Compare April 9, 2020 16:03
@c-bata c-bata changed the title Avoid duplicated import problems Avoid star imports Apr 9, 2020
@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Apr 9, 2020
@c-bata c-bata changed the title Avoid star imports Avoid using star imports Apr 9, 2020
@c-bata
Copy link
Member Author

c-bata commented Apr 9, 2020

All star-imports are removed.

(venv) $ git co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
(venv) $ git grep "import \*" | grep "\.py"
pkg/suggestion/v1alpha3/chocolate/base_chocolate_service.py:from pkg.suggestion.v1alpha3.internal.search_space import *
pkg/suggestion/v1alpha3/chocolate/base_chocolate_service.py:from pkg.suggestion.v1alpha3.internal.trial import *
pkg/suggestion/v1alpha3/hyperopt/base_hyperopt_service.py:from pkg.suggestion.v1alpha3.internal.search_space import *
pkg/suggestion/v1alpha3/hyperopt/base_hyperopt_service.py:from pkg.suggestion.v1alpha3.internal.trial import *
pkg/suggestion/v1alpha3/skopt/base_skopt_service.py:from pkg.suggestion.v1alpha3.internal.search_space import *
pkg/suggestion/v1alpha3/skopt/base_skopt_service.py:from pkg.suggestion.v1alpha3.internal.trial import *
(venv) $ git co fix-duplicated-imports
Switched to branch 'fix-duplicated-imports'
(venv) $ git grep "import \*" | grep "\.py"
(venv) $ 

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thanks @c-bata !
/lgtm
/cc @gaocegege

from pkg.suggestion.v1alpha3.internal.search_space import *
from pkg.suggestion.v1alpha3.internal.trial import *
from pkg.suggestion.v1alpha3.internal.search_space import (
INTEGER, DOUBLE, CATEGORICAL, DISCRETE, MAX_GOAL,
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in a better way to look more clean? we might have more constants. May be wrap this?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean that you want to import all constants, right?
Another solution is to define __all__ attribute like this revision. But as the discussion of #1133 (comment), it is known as an anti-pattern in Python.

Copy link
Member

Choose a reason for hiding this comment

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

@c-bata What do you think about creating file: constant.py under /internal/ folder ?
And insert all constant there? I think it will be easier to have all constant in one file

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds good 👍 @johnugeorge What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. SGTM

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 13, 2020
@c-bata c-bata changed the title Avoid using star imports Fix duplicated imports Apr 13, 2020
@johnugeorge
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 2a1a811 into kubeflow:master Apr 13, 2020
@c-bata c-bata deleted the fix-duplicated-imports branch April 26, 2020 07:38
sperlingxx pushed a commit to sperlingxx/katib that referenced this pull request Jul 9, 2020
* Avoid star-imports

* Add constants
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants