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

Ensure absolute imports #2789

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

johnnv1
Copy link
Member

@johnnv1 johnnv1 commented Feb 8, 2024

This patch bans relative imports, and moves all our relative imports to absolute imports using ruff

based on https://docs.astral.sh/ruff/settings/#lint_flake8-tidy-imports_ban-relative-imports

To reproduce:

  1. add the ruff tool.ruff.lint.flake8-tidy-imports rule to ban relative imports
  2. execute ruff run ./ --fix --unsafe-fixes

To be merged after #2788

@johnnv1 johnnv1 added the code heatlh 💊 Improvement the package code health label Feb 8, 2024
@johnnv1 johnnv1 added this to the kornia v0.7.2 milestone Feb 8, 2024
Copy link
Member

@edgarriba edgarriba left a comment

Choose a reason for hiding this comment

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

Worth to double check that the internal imports are really absolutely pointing to the deepest submodule possible. We can trim after once we make sure and control over the circular dependencies

from kornia.augmentation._2d.base import AugmentationBase2D, RigidAffineAugmentationBase2D
from kornia.augmentation import auto
from kornia.augmentation import container
from kornia.augmentation._2d import CenterCrop
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here makes sense wildcards and inside the sub module we add what to be imported by the all variable

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't understand what do you mean

from .sold2 import SOLD2, SOLD2_detector
from .sosnet import SOSNet
from .tfeat import TFeat
from kornia.feature.affine_shape import LAFAffineShapeEstimator
Copy link
Member

Choose a reason for hiding this comment

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

Maybe same here with wildcards

@johnnv1
Copy link
Member Author

johnnv1 commented Feb 9, 2024

Worth to double check that the internal imports are really absolutely pointing to the deepest submodule possible. We can trim after once we make sure and control over the circular dependencies

We should manually review each one for it, or do you know how to automate it?

@@ -5,12 +5,11 @@
import torch

from kornia.core import stack
from kornia.geometry.calibration.distort import distort_points, tilt_projection
from kornia.geometry.linalg import transform_points
from kornia.geometry.transform import remap
Copy link
Member

Choose a reason for hiding this comment

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

@johnnv1 just as random example, this import is not full absolute to where it’s actually implemented. I’m pretty sure there way more in code base

Copy link
Member Author

Choose a reason for hiding this comment

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

got it, needs to review/check each one

@edgarriba edgarriba mentioned this pull request Mar 11, 2024
14 tasks
@johnnv1 johnnv1 modified the milestones: kornia v0.7.2, kornia v0.7.3 Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code heatlh 💊 Improvement the package code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants