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

Use ruff #1994

Merged
merged 19 commits into from
Apr 15, 2024
Merged

Use ruff #1994

merged 19 commits into from
Apr 15, 2024

Conversation

adamjstewart
Copy link
Collaborator

@adamjstewart adamjstewart commented Apr 13, 2024

  • Configure ruff
  • Apply format changes
  • Fix linter errors
  • Update requirements files
  • Update GitHub Actions
  • Update docs
  • Update pre-commit
  • Update VS Code

Closes #1986

@adamjstewart adamjstewart added this to the 0.6.0 milestone Apr 13, 2024
@github-actions github-actions bot added datasets Geospatial or benchmark datasets models Models and pretrained weights testing Continuous integration testing samplers Samplers for indexing datasets trainers PyTorch Lightning trainers transforms Data augmentation transforms datamodules PyTorch Lightning datamodules dependencies Packaging and dependencies scripts Training and evaluation scripts labels Apr 13, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 13, 2024
"ms-python.python",
"ms-python.isort",
"ms-python.black-formatter",
"ms-python.vscode-pylance",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got rid of pylance since we aren't currently using it anywhere else. Also, haven't tested this file since I don't know how to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ashnair1 can you test this?

I'm wondering if we should remove mypy from pre-commit since it's so slow.

We could also add the pre-commit action so that fixes are automatically applied to PRs.

Copy link
Collaborator

@ashnair1 ashnair1 Apr 14, 2024

Choose a reason for hiding this comment

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

Tested the file and it works fine.

The slowness observed in the mypy hook occurs when there is a change in the additional dependencies as the isolated environment (where pre-commit runs the hooks) needs to be recreated. Once it's done, subsequent runs will be faster.

Incorporating the pre-commit action is a good idea. That way, new contributors won't have to worry about style. From a CI perspective, since we already have a Github action for checking mypy, we theoretically wouldn't need a mypy hook in the pre-commit config anymore.

On the flip side, having the mypy hook in the pre-commit config is useful for testing mypy when you don't have the latest versions of the libraries installed locally :)

"from torchgeo.trainers import ClassificationTask\n",
"from torchgeo.models import ResNet18_Weights"
"from torchgeo.models import ResNet18_Weights\n",
"from torchgeo.trainers import ClassificationTask"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like isort and pyupgrade did not have Jupyter notebook support

# tests
mypy==0.900
nbmake==1.3.3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't actually run any docs/style tests with the min version, no need to install those deps.

@@ -1,5 +1,4 @@
# tests
mypy==1.9.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved "mypy" under "style" in CI but forgot to move it in the requirements files.

), "You have selected an invalid country, please choose one of {}".format(
self.all_countries
)
), f"You have selected an invalid country, please choose one of {self.all_countries}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -293,12 +293,12 @@ def _extract(self) -> None:
features_path = os.path.join(self.root, self.file_dict["features"]["filename"])
extract_archive(features_path)

def plot(self, sample: dict[str, Tensor], subtitle: str | None = None) -> Figure:
def plot(self, sample: dict[str, Tensor], suptitle: str | None = None) -> Figure:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actual bug caught by pydocstyle!

transforms: a function/transform that takes input sample and its target as
entry and returns a transformed version
download: if True, download dataset and store it in the root directory
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lots of missing parameters in the docs

@@ -290,7 +291,7 @@ def _load_files(self, root: str) -> list[dict[str, Any]]:

images = imgs_no_subcat + imgs_subcat

files = [dict(image=img, label=l) for img, l in zip(images, labels)]
files = [dict(image=image, label=label) for image, label in zip(images, labels)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Linter complained about l being ambiguous (l vs. 1)

Copy link
Collaborator

@isaaccorley isaaccorley left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/style.yaml Show resolved Hide resolved
torchgeo/models/fcsiam.py Show resolved Hide resolved
@adamjstewart
Copy link
Collaborator Author

I actually plan to expand the number of rules (stricter pydocstyle) and make some other formatting changes (double -> single quotes) but will save that for another PR. So far, this PR simply replaces our current tools with a single, faster, less buggy tool.

@adamjstewart
Copy link
Collaborator Author

I've tried rerunning the minimum tests 5 or 6 times but no luck. Pretty sure codecov is dead: #1995

@isaaccorley
Copy link
Collaborator

Shall we merge w/o codecov passing?

@adamjstewart adamjstewart merged commit d9991ed into microsoft:main Apr 15, 2024
17 of 18 checks passed
@adamjstewart adamjstewart deleted the style/ruff branch April 15, 2024 21:46
@calebrob6
Copy link
Member

How fast is this now?

@adamjstewart
Copy link
Collaborator Author

Before

> time begin; black .; flake8; isort .; pydocstyle; pyupgrade --py310-plus $(find . -path ./docs/src -prune -o -name "*.py" -print); end
...
Executed in    7.18 secs    fish           external
   usr time   12.33 secs    1.30 millis   12.33 secs
   sys time    2.74 secs    9.48 millis    2.73 secs

After

> time begin; ruff check; ruff format; end
...
Executed in   83.13 millis    fish           external
   usr time   48.57 millis    0.17 millis   48.41 millis
   sys time   83.33 millis    2.75 millis   80.58 millis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets dependencies Packaging and dependencies documentation Improvements or additions to documentation models Models and pretrained weights samplers Samplers for indexing datasets scripts Training and evaluation scripts testing Continuous integration testing trainers PyTorch Lightning trainers transforms Data augmentation transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ruff
4 participants