chore(dev): Run cython-lint against codebase, add it to pre-commit

[MarcoGorelli]

Description

closes

closes #941

This PR adds cython-lint to pre-commit and fixes the flagged lint violations (mostly unused imports and variables)

Checklist

• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[mdboom]

Thanks. I'm a big fan of linting and pre-commits so I like this in general.

Unfortunately, many of the files that required changes here are autogenerated files, so next time we regenerate the files these violations will just come back.

I think we need to either:

• Fix these violations in cython-gen and cybind (the two generator tools that contribute these files, then regenerate and commit the result here.
• Ignore all auto-generated files in this check.

I think the first is the better long-term solution, but it will require some back-and-forth across the 3 projects.

[mdboom]

• Fix these violations in cython-gen and cybind (the two generator tools that contribute these files, then regenerate and commit the result here.

Actually, it looks like just cybind is implicated here -- no changes to cython-gen required, but point still stands.

[leofang]

Thanks, @MarcoGorelli! Big fan of your projects (including cython-lint)! 🙏

Unfortunately, in addition to the concern that Mike noted (that cuda-bindings code are mostly autogenerated and linting them makes little sense), we have a bigger issue, which is the legal aspect. Everywhere else in this repo accepts external contribution, except for files under the cuda_bindings/ folder 😅 because it does not have an OSS license. https://nvidia.github.io/cuda-python/cuda-bindings/latest/contribute.html.

Would you mind rebasing and purging all changes to cuda_bindings/?

[MarcoGorelli]

that's very kind, thank you!

sure, I reset the commit add excluded cuda_bindings

[leofang]

/ok to test 1fd64c8

[mdboom]

Thanks!

[github-actions]

Doc Preview CI
Preview removed because the pull request was closed or merged.

[rwgk]

In passing I noticed this error when adding the --verbose option:

pre-commit run --all-files --verbose

cython-lint..............................................................Passed
- hook id: cython-lint
- duration: 0.48s

Skipping file /home/rgrossekunst/forked/cuda-python/cuda_core/cuda/core/experimental/_device.pyx, as it cannot be parsed. Error: AttributeError("'_thread._local' object has no attribute 'cython_errors_listing_file'")

(There is no traceback.)

Maybe a bug in cython-lint?

[MarcoGorelli]

hi @rwgk

that means that Cython's parse_from_strings raised when parsing it

https://github.com/MarcoGorelli/cython-lint/blob/b9cf4701d98b34a7d5e7adbd9e446f97dd9700a4/cython_lint/cython_lint.py#L403-L412

I'd suggest reporting this to Cython itself

[rwgk]

For easy reference, this is the existing cython-lint code:

    except Exception as exp:  # pragma: no cover
        # If Cython can't parse this file, just skip it.
        print(
            f"Skipping file {filename}, as it cannot be parsed. Error: {exp!r}",
        )
        raise CythonParseError from exp

Unfortunately the original error and the traceback don't appear. → I'm giving up (and I think most people will give up) right there.

WDYT about (totally untested and with a huge amount of guessing):

    except Exception as exp:  # pragma: no cover
        # If Cython can't parse this file, just skip it.
        sys.stderr.flush()
        print(
            f"Skipping file {filename}, as it cannot be parsed. Error: {exp!r}",
        )
        traceback.print_exc(file=sys.stdout)
        sys.stdout.flush()
        raise CythonParseError from exp

Will that surface the cython error? (That's what I'm hoping to achieve.)

I'm also wondering: _device.pyx builds successfully (the output pasted before is with cuda-python main as-is). Why does the parsing fail with cython-lint?

[MarcoGorelli]

sure, i've reported it here: cython/cython#7235