Add cython-lint configuration. #5439

bdice · 2023-05-24T21:57:52Z

This PR adds a configuration for cython-lint. It eliminates a lot of unused imports and unused variables from the Cython code.

bdice · 2023-05-24T22:03:09Z

pyproject.toml

@@ -9,3 +9,7 @@ ignore-words-list = "inout,numer,startd,couldn,referr"
 builtin = "clear"
 # disable warnings about binary files and wrong encoding
 quiet-level = 3
+
+[tool.cython-lint]
+max-line-length = 999


I'd like to re-enable E501 with a reasonable line length limit (following our flake8/black conventions) in a follow-up PR. Wrapping lines takes a little more manual effort than I wanted to put in for this first pass.

bdice · 2023-05-24T22:03:52Z

python/cuml/ensemble/randomforest_common.pyx

@@ -347,7 +345,7 @@ class BaseRandomForestModel(Base):
                              fil_sparse_format, threshold=0.5,
                              output_class=False,
                              predict_proba=False) -> CumlArray:
-        _, n_rows, n_cols, dtype = \
+        _, _, _, _ = \


Is this call to input_to_cuml_array doing anything? Its results are not used but it might be doing some error checking / validation?

~~It will set the classe's input_dtype. Getting rid of unexpected side-effects like that is one of my refactoring goals.~~ Raelized it's not involving the class, might not do anything.

This is doing nothing. The CumlArray conversion happens in the FIL call. I suspect this is an artifact of a time before FIL was used for inference here.

@wphicks Can you push changes or file an issue if you'd like to see a change here? I don't have enough familiarity with this code, unfortunately.

I pushed a commit, removing the affected line.

csadorf

Love it. ❤️

Can you add a note to https://github.com/rapidsai/cuml/blob/branch-23.06/CONTRIBUTING.md#summary-of-pre-commit-hooks ?

pyproject.toml

bdice · 2023-05-24T22:25:51Z

Style checks are failing because of incorrect dependency versions. I opened #5440 to resolve it.

bdice · 2023-06-02T14:41:25Z

@csadorf I resolved merge conflicts on this PR. Can you re-approve it now that you have Python code owner privileges?

csadorf · 2023-06-07T14:38:16Z

/merge

bdice · 2023-06-28T16:02:49Z

python/cuml/manifold/umap.pyx

-from cuml.manifold.simpl_set import fuzzy_simplicial_set, \
-    simplicial_set_embedding


Deleting the unused import of simplicial_set_embedding broke test_simpl_set.py. Is this meant to be a public API exposed from cuml.manifold.umap? Do we need to worry about other unused imports removed in this PR that are meant to be exposed as public APIs? If so, the proper mechanism is to define __all__ = ["simplicial_set_embedding", ...].

cc: @wphicks @dantegd @csadorf

We will have to perform a more thorough check on which symbols should be included in the public API (and either excluded from linting or explicitly added to __all__). I will take care of that.

Thanks @csadorf!

If we want to merge this PR sooner, we could disable the linter checks for unused Python imports (unused Cython cimports should all be safe to remove).

Yes, please go ahead with that.

I filed an issue about this: MarcoGorelli/cython-lint#79

cython-lint also doesn't detect usage in __all__, so that isn't a viable solution here. I filed a second issue: MarcoGorelli/cython-lint#80

For now, I added these imports back with # no-cython-lint in d773305. That should resolve the problem. I reviewed the changes in this PR for other possible public API breaks but I didn't see any candidates except for #5439 (comment).

I've added a comment that explains the need for the markers there.

bdice · 2023-06-30T17:53:36Z

python/cuml/explainer/kernel_shap.pyx

@@ -26,10 +25,8 @@ from cuml.internals.input_utils import input_to_cupy_array
 from cuml.explainer.base import SHAPBase
 from cuml.explainer.common import get_cai_ptr
 from cuml.explainer.common import model_func_call
-from cuml.explainer.common import output_list_shap_values


Is this a public API? If so we need to add it back with # no-cython-lint since it is an unused import. Otherwise no action is needed.

This is the only questionable case I saw, all the other unused imports are clearly not meant to be public APIs.

I think it is safe to assume that it is not.

csadorf

Just reviewed this change set once more with special attention to the removed imports. That looks all good to me.

I've also pushed three commits with some changes, but once tests pass, this should be ready for merge.

csadorf · 2023-06-30T21:44:26Z

python/cuml/manifold/umap.pyx

-from cuml.manifold.simpl_set import fuzzy_simplicial_set, \
-    simplicial_set_embedding


I've added a comment that explains the need for the markers there.

csadorf · 2023-06-30T21:45:01Z

python/cuml/manifold/umap_utils.pyx

@@ -128,5 +128,5 @@ def find_ab_params(spread, min_dist):
    yv = np.zeros(xv.shape)
    yv[xv < min_dist] = 1.0
    yv[xv >= min_dist] = np.exp(-(xv[xv >= min_dist] - min_dist) / spread)
-    params, covar = curve_fit(curve, xv, yv)
+    params, _covar = curve_fit(curve, xv, yv)


Suggested change

params, _covar = curve_fit(curve, xv, yv)

params, _ = curve_fit(curve, xv, yv)

Should do it, right?

I went ahead and applied that change.

csadorf · 2023-06-30T21:51:57Z

python/cuml/ensemble/randomforest_common.pyx

@@ -347,7 +345,7 @@ class BaseRandomForestModel(Base):
                              fil_sparse_format, threshold=0.5,
                              output_class=False,
                              predict_proba=False) -> CumlArray:
-        _, n_rows, n_cols, dtype = \
+        _, _, _, _ = \


I pushed a commit, removing the affected line.

bdice · 2023-06-30T22:12:42Z

@csadorf Thanks for seeing this through! Your final changes LGTM.

csadorf · 2023-06-30T22:55:51Z

Note that the wheeltest failures are a known and unrelated issue. They should be resolved by Monday.

bdice added 3 commits May 24, 2023 15:39

Add cython-lint configuration.

4746ed8

Add config to exclude line length errors.

29d2568

Apply cython-lint changes.

cffee7b

bdice requested a review from a team as a code owner May 24, 2023 21:57

github-actions bot added the Cython / Python Cython or Python issue label May 24, 2023

bdice commented May 24, 2023

View reviewed changes

csadorf approved these changes May 24, 2023

View reviewed changes

pyproject.toml Show resolved Hide resolved

Update docs.

5cb7975

bdice self-assigned this Jun 2, 2023

Merge remote-tracking branch 'upstream/branch-23.08' into cython-lint

481abad

bdice added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jun 2, 2023

bdice added 3 commits June 2, 2023 11:59

Merge branch 'branch-23.08' into cython-lint

acb6799

Apply fixes from rapids-dependency-file-generator.

afb9b0d

Merge branch 'branch-23.08' into cython-lint

ac2ddbe

csadorf approved these changes Jun 5, 2023

View reviewed changes

Merge branch 'branch-23.08' into cython-lint

ce6c16f

Merge branch 'branch-23.08' into cython-lint

225b7c5

bdice commented Jun 28, 2023

View reviewed changes

csadorf assigned csadorf and unassigned bdice Jun 28, 2023

bdice commented Jun 30, 2023

View reviewed changes

bdice and others added 4 commits June 30, 2023 13:00

Add back public APIs with no-cython-lint.

d773305

Merge branch 'branch-23.08' into cython-lint

44fb543

Remove obsolete input_to_cuml_array conversion in BaseRandomForestModel.

92e8a8f

Drop unneeded variable name.

e324d38

Add comment that explains need for no-cython-lint markers.

1cfe86c

csadorf approved these changes Jun 30, 2023

View reviewed changes

rapids-bot bot merged commit e350b99 into rapidsai:branch-23.08 Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cython-lint configuration. #5439

Add cython-lint configuration. #5439

bdice commented May 24, 2023

bdice May 24, 2023

bdice May 24, 2023

csadorf May 24, 2023 •

edited

Loading

wphicks May 25, 2023

bdice Jun 2, 2023

csadorf Jun 30, 2023

csadorf left a comment

bdice commented May 24, 2023

bdice commented Jun 2, 2023

csadorf commented Jun 7, 2023

bdice Jun 28, 2023 •

edited

Loading

csadorf Jun 29, 2023

bdice Jun 29, 2023

bdice Jun 29, 2023

csadorf Jun 29, 2023

bdice Jun 29, 2023

bdice Jun 29, 2023

bdice Jun 30, 2023

csadorf Jun 30, 2023

bdice Jun 30, 2023 •

edited

Loading

csadorf Jun 30, 2023

csadorf left a comment

csadorf Jun 30, 2023

csadorf Jun 30, 2023

csadorf Jun 30, 2023

csadorf Jun 30, 2023

bdice commented Jun 30, 2023

csadorf commented Jun 30, 2023

		from cuml.manifold.simpl_set import fuzzy_simplicial_set, \
		simplicial_set_embedding

	params, _covar = curve_fit(curve, xv, yv)
	params, _ = curve_fit(curve, xv, yv)

Add cython-lint configuration. #5439

Add cython-lint configuration. #5439

Conversation

bdice commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

bdice commented May 24, 2023

bdice commented Jun 2, 2023

csadorf commented Jun 7, 2023

bdice Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice commented Jun 30, 2023

csadorf commented Jun 30, 2023

csadorf May 24, 2023 •

edited

Loading

bdice Jun 28, 2023 •

edited

Loading

bdice Jun 30, 2023 •

edited

Loading