Ruff violation icn001 fix by Shaheer-Ahmd · Pull Request #15899 · astropy/astropy

Shaheer-Ahmd · 2024-01-16T20:36:02Z

Description

This PR fixes the violation of RUFF rule ICN001

unconventional-import-alias (ICN001)

Derived from the flake8-import-conventions linter.

Fix is sometimes available.

What it does

Checks for imports that are typically imported using a common convention,
like import pandas as pd, and enforces that convention.

Why is this bad?

Consistency is good. Use a common convention for imports to make your code
more readable and idiomatic.

For example, import pandas as pd is a common
convention for importing the pandas library, and users typically expect
Pandas to be aliased as pd.

Example

import pandas

Use instead:

import pandas as pd

This pull request is to address ...

Fixes #14818

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-01-16T20:36:32Z

nstarman · 2024-01-16T21:53:28Z

Hi @Shaheer-Ahmd. Thanks for opening the PR! I like the rule ICN001 and think it's worth implementing. This PR is quite ambitious in that it fixes ICN001 everywhere in Astropy. In Ensure compliance with more Ruff rules we actually recommend breaking these large PRs into a few smaller ones. That way not everyone in Astropy has to sign off on an omnibus PR, instead you can get these PRs in faster by needing only 1 or 2 people to approve for each PR.

eerovaher · 2024-01-19T13:54:56Z

We shouldn't write weird code just to satisfy a linter, but there is nothing strange about import numpy as np and there is nothing strange about from {module} import {function} either.

dhomeier · 2024-01-19T14:04:51Z

No, but would anyone have chosen to alias it to something like get_numpy_include in normal circumstances?

eerovaher · 2024-01-19T14:34:27Z

We could just use from numpy import get_include.

mhvk · 2024-01-19T14:55:04Z

I'm fine with the current state, somewhat preferring the renaming (all this may become irrelevant anyway, as surely at some point someone is going to suggest we switch to meson...)

mhvk

I think this is now all fine! Thanks, @Shaheer-Ahmd

p.s. Very small suggestion for future PRs: push commits in batches so one avoids running CI for every commit separately.

Shaheer-Ahmd · 2024-01-20T10:46:31Z

Is this merge-ready then? Any other changes?

dhomeier · 2024-01-20T14:13:42Z

If everyone agrees on the custom alias, that should be ready, but needs squeezing. If you want to do it yourself, reducing to 2-3 commits should probably be ok.
And if we stick with the full set of enforced aliases rather than setting our own, they should be added to the documentation (see #15899 (comment)).
Or perhaps add a link there to the Ruff docs, as I suspect this may become a moving target.
The failed cron job is probably unrelated, but not certain...

Shaheer-Ahmd · 2024-01-20T15:50:58Z

By the "full set of enforced aliases" you mean
{"altair": "alt", "matplotlib": "mpl", "matplotlib.pyplot": "plt", "numpy": "np", "pandas": "pd", "seaborn": "sns", "tensorflow": "tf", "tkinter": "tk", "holoviews": "hv", "panel": "pn", "plotly.express": "px", "polars": "pl", "pyarrow": "pa"}
these? (default aliases in ruff docs)
and for the squeezing, should I make a new PR with larger but less in number commits and link it here? (sorry for asking too many questions, I am a newbie here)

dhomeier · 2024-01-20T22:23:47Z

By the "full set of enforced aliases" you mean {"altair": "alt", "matplotlib": "mpl", "matplotlib.pyplot": "plt", "numpy": "np", "pandas": "pd", "seaborn": "sns", "tensorflow": "tf", "tkinter": "tk", "holoviews": "hv", "panel": "pn", "plotly.express": "px", "polars": "pl", "pyarrow": "pa"} these? (default aliases in ruff docs)

That is the full list, but since most of these are not imported anywhere within astropy, and many likely never will be, it should be enough to add pandas as pd to our list, and then link to https://docs.astral.sh/ruff/settings/#flake8-import-conventions-aliases for reference – that way we don't need to worry about updating whenever Ruff developers discover a new alias considered conventional.

and for the squeezing, should I make a new PR with larger but less in number commits and link it here? (sorry for asking too many questions, I am a newbie here)

No, squashing (sorry, my typo before) is done on your existing branch, as described in
https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#how-to-squash
One commit for units, one for setup_package.py and one for everything else would probably make sense, though I am not sure if you can combine them out of order...
We can also squash automatically on merge, but that way everything would be stuffed into a single commit.

…est_masking

…mport in mpl_normalize and updated docs

Shaheer-Ahmd · 2024-01-21T22:41:45Z

squashed and updated the docs.

dhomeier · 2024-01-21T23:40:16Z

Thanks a lot, looking great now! Apparently there is a new deprecation in numpy-dev, but that's nothing that needs to concern this PR.

eerovaher · 2024-01-22T14:01:23Z

    import numpy as np
    import matplotlib as mpl
    import matplotlib.pyplot as plt
+    import pandas as pd


If anybody ever deviates from the naming conventions ICN001 enforces then the Ruff hook in pre-commit will clearly state where in the code the violation occurred and what name it expects to be used. It is even possible to have Ruff fix the violations automatically. I don't think this text in the developer documentation is informative anymore.

Personally I prefer to know what I am expected to write before a pre-commit tool tells me I've done it wrong and how to fix it. Besides, we are still seeing issues with contributors who are unable to set up their pre-commit locally.
If we have section in the docs stating in some detail what import styles Ruff is or is going to enforce via isort, flake8 conventions and what else, that might replace this explicit list, but currently I don't see such a reference. Opinions may vary on this.
Also it is a good reminder of what plt.figure stands for when encountered anywhere in the code without even having to look at the imports.

I'm not sure if people are actually having problems installing pre-commit. I consider it much more likely that they haven't followed our instructions to install pre-commit simply because they have not read our developer documentation at all, in which case nothing we write there is going to help them.

I'd rather not write down any specific rules that Ruff would be enforcing anyways because I believe the shorter our developer documentation is the more people might actually read it.

I suspect similarly. I've installed pre-commit on everything from a Python env on my phone (bad idea, don't do dev work on the phone) to a compute cluster. I've never once run into a problem with the pip install except when I didn't have git on a machine.

Currently the docs do not even state that you must install and activate pre-commit before you are allowed to push anything upstream, so that would need to be changed along the way. And then how would you handle cases when a series of commits fail in the CI pre-commit, you ask the contributor to run pre-commit locally and they state they have it installed, yet it is apparently allowing invalid commits? I thought the point of all the pre-commit tools was to lower the threshold for new contributors, but that is not always reflected by my experience – apparently it is not that easy for everyone, or are those just some freak cases of exotic Windows installations?

Is this conversation blocking merge? Should I wait?

A first-time contributor who needs multiple commits to get the code style checks to pass has proven themselves to be untrustworthy when it comes to code style and the related tooling.

It is technically possible to get multiple commits to pass and then fail pre-commit. But this requires rebases and other fancy stuff that then skip the "pre-commit".
Our CI run of pre-commit should tell any contributor exactly what to fix. And the comment pre-commit autofix can take care of many stuff server side.
Installing pre-commit locally AFAIK is always as easy as pip install pre-commit; cd <git repo>; pre-commit install and makes their life much easier.

Is this conversation blocking merge? Should I wait?

I think we should update some of our developer documentation, but doing that properly is beyond the scope here and shouldn't block merging.

Indeed, this is moving to an entirely different topic and certainly needs more discussion.
The PR was already squashed; I probably would not have added the as np imports just to rename them later, but I think our commit history can live with that. ;-)

To conclude this part of the discussion here, in my experience it simply is not straightforward for every first-time contributor to parse a report like https://results.pre-commit.ci/run/github/2081289/1703509121.Z6WkqI4KQVm9d2te2s3qhg and make the required fixes, and if I see such commits coming in even after mentioning the use of pre-commit, I am still hesitant to label anyone as "untrustworthy" without knowing myself about any possible pitfalls on a given platform. For which we don't seem to be able to provide that much guidance e.g. for the various Windows platforms.

pllim · 2024-01-22T20:07:45Z

Thanks, all!

dhomeier · 2024-01-22T20:15:23Z

Hmm, maybe I misunderstood the interpretation of the "rebase or squash necessary?" checkmark...

pllim · 2024-01-22T20:16:56Z

@dhomeier , what do you mean?

dhomeier · 2024-01-22T20:19:29Z

I normally check the marks in the sense of "no further action required" since @Shaheer-Ahmd had already squashed down to 3 commits locally. But that list is only used very inconsistently anyway.

pllim · 2024-01-22T20:20:50Z

Oh... I only noticed that the box in the origin post was not checked. 😬

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

dhomeier · 2024-01-22T20:23:41Z

But that box does not mean "if unchecked the PR will always be squashed on merge"? We surely have smaller PRs merged with more commits...

pllim · 2024-01-22T20:51:08Z

Sorry, it is my bad. I didn't know you wanted it not squashed. I made a call at merge time and apparently I made the wrong call. I will take full responsibility.

But seems overkill to revert and I don't want to force push to main. I will be more careful next time!

dhomeier · 2024-01-22T21:04:57Z

No problem, I think it can fit into a single commit – I meant to reply to Eero that it was already squashed, but did not get to it while the discussion on pre-commit was still going on. Thanks everyone!

Shaheer-Ahmd requested review from Cadair, WilliamJamieson, astrofrog, dhomeier, larrybradley, matteobachetti, mcara, nstarman and taldcroft as code owners January 16, 2024 20:36

github-actions Bot added testing wcs table time io.ascii units utils io.misc stats convolution unified-io visualization visualization.wcsaxes timeseries dev-automation labels Jan 16, 2024

pllim added this to the v6.1.0 milestone Jan 16, 2024

pllim added no-changelog-entry-needed Refactoring labels Jan 16, 2024

pllim requested a review from eerovaher January 16, 2024 21:46

mhvk approved these changes Jan 19, 2024

View reviewed changes

Comment thread astropy/io/misc/pandas/connect.py Resolved

pllim reviewed Jan 19, 2024

View reviewed changes

Comment thread astropy/units/astrophys.py Resolved

pllim reviewed Jan 19, 2024

View reviewed changes

Comment thread astropy/visualization/wcsaxes/utils.py Resolved

Shaheer-Ahmd force-pushed the ruff-violation-ICN001-fix branch from 851a2a4 to fabc216 Compare January 21, 2024 22:14

Shaheer-Ahmd added 2 commits January 22, 2024 03:21

initial ICN001 fix and removed it from ignore list

c5d3cd3

changed numpy import alias in setup_packages and used minversion in t…

c55a0c7

…est_masking

Shaheer-Ahmd force-pushed the ruff-violation-ICN001-fix branch from fabc216 to ee641b5 Compare January 21, 2024 22:23

moved numpy import to the top in units, removed leftover matplotlib i…

1a68f4a

…mport in mpl_normalize and updated docs

Shaheer-Ahmd force-pushed the ruff-violation-ICN001-fix branch from ee641b5 to 1a68f4a Compare January 21, 2024 22:39

dhomeier approved these changes Jan 22, 2024

View reviewed changes

eerovaher reviewed Jan 22, 2024

View reviewed changes

pllim merged commit aeb53d5 into astropy:main Jan 22, 2024

Conversation

Shaheer-Ahmd commented Jan 16, 2024

Description

unconventional-import-alias (ICN001)

What it does

Why is this bad?

Example

Uh oh!

github-actions Bot commented Jan 16, 2024 • edited by dhomeier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Jan 16, 2024

Uh oh!

eerovaher commented Jan 19, 2024

Uh oh!

dhomeier commented Jan 19, 2024

Uh oh!

eerovaher commented Jan 19, 2024

Uh oh!

mhvk commented Jan 19, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shaheer-Ahmd commented Jan 20, 2024

Uh oh!

dhomeier commented Jan 20, 2024

Uh oh!

Shaheer-Ahmd commented Jan 20, 2024

Uh oh!

dhomeier commented Jan 20, 2024

Uh oh!

Shaheer-Ahmd commented Jan 21, 2024

Uh oh!

dhomeier commented Jan 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhomeier Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 22, 2024

Uh oh!

dhomeier commented Jan 22, 2024

Uh oh!

pllim commented Jan 22, 2024

Uh oh!

dhomeier commented Jan 22, 2024

Uh oh!

pllim commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhomeier commented Jan 22, 2024

Uh oh!

pllim commented Jan 22, 2024

Uh oh!

dhomeier commented Jan 22, 2024

Uh oh!

Reviewers

Uh oh!

github-actions Bot commented Jan 16, 2024 •

edited by dhomeier

Loading

dhomeier Jan 22, 2024 •

edited

Loading

pllim commented Jan 22, 2024 •

edited

Loading