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 pyproject.toml instead of setup.cfg, pytest.ini & requirements.txt and other Python related config files #276

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Feb 26, 2024

Modern Python is slowly adopting pyproject.toml as a single place to define a Python package and related tool settings like black for formatting, flake8 for linting and pytest for testing.

Lets try to move as much of the Python-related project configuration to this file and unclutter the root of the repo.

This PR does:

  • Merge requirements*.txt, pytest.ini and setup.cfg files into pyproject.toml
  • Update shell scripts and Dockerfiles to use pyproject.toml
  • Introduce a pre-commit configuration file
  • Add a GitHub actions workflow for running pre-commit also in the CI
  • Apply/fix pre-commit checks to all files in the repo:
    • Harmonize file endings (single empty line)
    • Remove trailing whitespaces
    • Apply formatting of all Python code (also in Jupyter notebooks)
  • Remove obsolete CI jobs from .gitlab-ci.yml

@stv0g stv0g marked this pull request as draft February 26, 2024 11:07
@stv0g stv0g self-assigned this Feb 26, 2024
@stv0g
Copy link
Contributor Author

stv0g commented Feb 27, 2024

I am interested in your opinions @leonardocarreras and @m-mirz.
What do you think about consolidate all the Python related project files in pyproject.toml

@m-mirz
Copy link
Contributor

m-mirz commented Feb 27, 2024

Good idea 👍

@leonardocarreras
Copy link
Collaborator

I think it is a great idea 🙌🏻

@stv0g stv0g force-pushed the pyproject branch 4 times, most recently from 16d172f to 0b2b038 Compare February 29, 2024 16:43
@stv0g stv0g marked this pull request as ready for review February 29, 2024 16:45
@stv0g
Copy link
Contributor Author

stv0g commented Feb 29, 2024

This is ready for review :)

I've also added pre-commit as a tool to perform several checks against changes which are staged for commit. Such as removing trailing whitespaces, checking JSON, YAML and XML syntax and Python / C++ code formatting.

While doing so, I realized that much of the Python code was ugly formatted.
We are now using the de-facto standard Python formatter Black.
It even can format Python code with in Jupyter notebooks :)

@stv0g
Copy link
Contributor Author

stv0g commented Mar 1, 2024

@m-mirz I guess the SonarCloud warnings are false-positives. Its at least not code which I've touched.

m-mirz
m-mirz previously approved these changes Mar 1, 2024
stv0g and others added 12 commits March 1, 2024 17:43
python: pick 266daa8f wip: villas: Minor fixes and tweaks for VillasInterface

Signed-off-by: Steffen Vogel <[email protected]>
Signed-off-by: Steffen Vogel <[email protected]>
Remove trailing whitespaces
Harmonize file endings to single empty line

Signed-off-by: Steffen Vogel <[email protected]>
Signed-off-by: Steffen Vogel <[email protected]>
Signed-off-by: Steffen Vogel <[email protected]>
Signed-off-by: Steffen Vogel <[email protected]>
@stv0g
Copy link
Contributor Author

stv0g commented Mar 4, 2024

Hi @martinmoraga @leonardocarreras @dinkelbachjan,

Markus has reviewed and approved this change. However, we would like to find more approvals before going ahead and merging it.

Do you have a second to look at this change?

pyproject.toml Outdated Show resolved Hide resolved
martinmoraga
martinmoraga previously approved these changes Mar 4, 2024
pip3 install -r requirements.txt
pip3 install --upgrade setuptools
pip3 install --upgrade wheel
pip3 install .[test,docs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here looks like we are doing a different thing, not only installing dependencies but also the package in the currently active folder (dpsim). Is this the intended behaviour?

Copy link
Contributor

@m-mirz m-mirz Mar 6, 2024

Choose a reason for hiding this comment

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

Actually, this looks fine to me @stv0g @leonardocarreras

pip3 install -r requirements.txt
pip3 install --upgrade setuptools
pip3 install --upgrade wheel
pip3 install .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason for the difference here about only pip3 install . vs .[test,docs] in the packaging/Shell/install-fedora-deps-dev.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

@stv0g Doesn't this install the dpsim package as well and not just deps?

@@ -24,8 +24,7 @@ apt-get -y install \
libgraphviz-dev \
libsundials-dev

pip3 install --user -r requirements.txt
pip3 install --user -r requirements-jupyter.txt
pip3 install --user .
Copy link
Collaborator

@leonardocarreras leonardocarreras Mar 4, 2024

Choose a reason for hiding this comment

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

Any particular reason for the difference here about only pip3 install . vs .[test,docs] in the packaging/Shell/install-fedora-deps-dev.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

@stv0g Here I would also expect only the installation of deps and not the dpsim package.

packaging/Shell/install-fedora-deps.sh Show resolved Hide resolved
setup.py Show resolved Hide resolved
packaging/Docker/Dockerfile.dev-minimal Outdated Show resolved Hide resolved
Copy link
Collaborator

@leonardocarreras leonardocarreras left a comment

Choose a reason for hiding this comment

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

Hi @stv0g! Thanks for the extra formatting, I think the code will look a lot more standardized now... From the side of the code it looks good to me in general, I would like some more opinions still on the how we do...

@leonardocarreras
Copy link
Collaborator

Side topic: Apparently, the more changes are here, the more Sonar complains and demands higher quality from reformatted existing code...

stv0g added 4 commits March 6, 2024 09:02
Signed-off-by: Steffen Vogel <[email protected]>
It refers to a file named requirements-jupyter.txt which
does not exist for quite some time now..

Also the file was broken by a missing line continuation..

Signed-off-by: Steffen Vogel <[email protected]>
Signed-off-by: Steffen Vogel <[email protected]>
@stv0g stv0g dismissed stale reviews from leonardocarreras, martinmoraga, and m-mirz via 35fa11c March 6, 2024 08:26
Copy link

sonarqubecloud bot commented Mar 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5 Security Hotspots
E Security Review Rating on New Code (required ≥ A)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@stv0g stv0g requested a review from m-mirz March 6, 2024 15:09
@m-mirz
Copy link
Contributor

m-mirz commented Apr 12, 2024

@stv0g @leonardocarreras I think this can be merged after the question regarding pip install have been resolved.

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.

4 participants