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

Move python tool configurations to pyproject.toml, and add the python 3.11 classifier. #9112

Merged
merged 4 commits into from
May 5, 2023
Merged

Move python tool configurations to pyproject.toml, and add the python 3.11 classifier. #9112

merged 4 commits into from
May 5, 2023

Conversation

UriyaHarpeness
Copy link
Contributor

@UriyaHarpeness UriyaHarpeness commented May 2, 2023

  • Add missing Programming Language :: Python :: 3.11 classifier for the python package.
  • Move pylint config to the pyproject.toml and delete .pylintrc.
  • Move mypy config to the pyproject.toml and delete setup.cfg.
  • Improve lint_python.py:
    • Reuse and readability.
    • Stop using soon-to-be-deprecated epylint.
    • Use the flags from the pyproject.toml instead of passing them through the CLI.

@trivialfis trivialfis requested a review from hcho3 May 2, 2023 09:54
@UriyaHarpeness
Copy link
Contributor Author

Hey @trivialfis @hcho3,
While on this subject, I see some flags are being passed in https://github.com/dmlc/xgboost/blob/master/tests/ci_build/lint_python.py, can these flags be removed if they already exist in the pyproject.toml? With perhaps changing the CWD when running the linting script?
It's a shame to have several copies that could get misaligned.

@trivialfis
Copy link
Member

trivialfis commented May 2, 2023

For mypy, it should work out of the box. pylint however, is a bit more messy as we haven't upgraded the configuration for a while, the file you referenced is based on some really old and deprecated features from pylint. I can try to upgrade the configuration later.

@UriyaHarpeness
Copy link
Contributor Author

@trivialfis please let me try to do it myself first, will do so very soon.

@trivialfis
Copy link
Member

Thank you for the nice work! Let me know if there's anything I can help.

@UriyaHarpeness
Copy link
Contributor Author

@trivialfis am I free to use the builtin library pathlib? Would also help simplify this linting script.

@trivialfis
Copy link
Member

yup, whatever that comes with python.

@hcho3
Copy link
Collaborator

hcho3 commented May 2, 2023

Thanks for working on updating the linting script. Let me know if there's anything I can help.

@UriyaHarpeness
Copy link
Contributor Author

UriyaHarpeness commented May 3, 2023

@hcho3 @trivialfis I think I'm done here, locally everything works.
The only thing I would've maybe done is split the actual linting script and it's configurations, currently it's mixed as seen below:

...
        mypy_results = [
            run_mypy(path)
            for path in [
                # core
                "python-package/",
                # demo
                "demo/json-model/json_parser.py",
                "demo/guide-python/external_memory.py",
                "demo/guide-python/cat_in_the_dat.py",
...

I am very willing to do so, let me know if it's acceptable by you and I shall change it.

PS. The readthedocs issue is unrelated and in progress by you guys: #9115.

@UriyaHarpeness
Copy link
Contributor Author

@hcho3 @trivialfis poke.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Very nice! Now the repository is much more consistent and unified, along with the linting code being cleaned up.

One minor issue mentioned in the comment, otherwise looks good to me.

tests/ci_build/lint_python.py Outdated Show resolved Hide resolved
@UriyaHarpeness
Copy link
Contributor Author

I think the PR is done now, I'm pretty happy with it.
Now, if I'm the one that should merge it just give me your approvals and I shall do so, otherwise - please go ahead and merge if it's to your liking.
Thanks @trivialfis @hcho3.

@trivialfis trivialfis merged commit a075aa2 into dmlc:master May 5, 2023
@trivialfis
Copy link
Member

thank you for your nice work on cleaning up the configuration and helper scripts!

@UriyaHarpeness UriyaHarpeness deleted the feature/move-python-tool-configuration-to-pyproject-toml branch May 6, 2023 17:36
@UriyaHarpeness
Copy link
Contributor Author

My pleasure! glad I could contribute.
@trivialfis @hcho3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants