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

Add ruff to the testing #421

Merged
merged 5 commits into from
Jan 29, 2025
Merged

Add ruff to the testing #421

merged 5 commits into from
Jan 29, 2025

Conversation

cclauss
Copy link
Collaborator

@cclauss cclauss commented Jan 23, 2025

Copy link
Collaborator

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Can we keep the existing style/pytest files as they were rather than moving everything to pyproject.toml?

.github/workflows/main.yml Outdated Show resolved Hide resolved
@cclauss
Copy link
Collaborator Author

cclauss commented Jan 27, 2025

From validate-pyproject README:

With the approval of PEP 517 and PEP 518, the Python community shifted towards a strong focus on standardization for packaging software, which allows more freedom when choosing tools during development and make sure packages created using different technologies can interoperate without the need for custom installation procedures.

This shift became even more clear when PEP 621 was also approved, as a standardized way of specifying project metadata and dependencies.

PEP 621 – Storing project metadata in pyproject.toml was approved in 2020.

@cclauss cclauss force-pushed the lint-with-ruff branch 2 times, most recently from b48151c to e491531 Compare January 28, 2025 11:11
@mtrofin
Copy link
Collaborator

mtrofin commented Jan 28, 2025

From validate-pyproject README:

With the approval of PEP 517 and PEP 518, the Python community shifted towards a strong focus on standardization for packaging software, which allows more freedom when choosing tools during development and make sure packages created using different technologies can interoperate without the need for custom installation procedures.

This shift became even more clear when PEP 621 was also approved, as a standardized way of specifying project metadata and dependencies.

PEP 621 – Storing project metadata in pyproject.toml was approved in 2020.

Seems reasonable to move to pyproject.toml. @boomanaiden154, is there a scenario that you're concerned with, that would be broken, or is this (more likely, PR #423) good to go (once the bots are clear)?

@cclauss cclauss force-pushed the lint-with-ruff branch 2 times, most recently from 31dba73 to 31ec4b8 Compare January 28, 2025 17:41
Copy link
Collaborator

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

@cclauss You need to update Pipfile.lock too.

@cclauss
Copy link
Collaborator Author

cclauss commented Jan 28, 2025

Unfortunately pipenv will not update Pipfile.lock for me on my M1 Mac.

Natively and inside Docker a similar error appears:
ERROR: Could not find a version that satisfies the requirement dm-reverb==0.14.0

https://pypi.org/project/dm-reverb/#installation is Linux only but in the Dockerfile container I am running on Linux.

Is it because I am on ARM and not Intel?


11.57 Collecting decorator==5.1.1 (from -r
11.57 /tmp/pipenv-gmvmcczu-requirements/pipenv-_e9n8ljz-hashed-reqs.txt (line 6))
11.57   Downloading decorator-5.1.1-py3-none-any.whl (9.1 kB)
11.57 ERROR: Could not find a version that satisfies the requirement dm-reverb==0.14.0
11.57 (from versions: none)
11.57 ERROR: No matching distribution found for dm-reverb==0.14.0
11.57 ERROR: Couldn't install package: {}
11.57  Package installation failed...
------
Dockerfile:5
--------------------
   3 |     WORKDIR /ml-compiler-opt
   4 |     COPY . .
   5 | >>> RUN pip install pipenv && pipenv sync --system && pipenv --clear
   6 |
   7 |     WORKDIR /ml-compiler-opt/compiler_opt/tools
--------------------
ERROR: failed to solve: process "/bin/sh -c pip install pipenv && pipenv sync --system && pipenv --clear" did not complete successfully: exit code: 1

@cclauss
Copy link
Collaborator Author

cclauss commented Jan 28, 2025

Is it because I am on ARM and not Intel?

Yes, it is...

GitHub Action...

name: dm-reverb
on:
  push:
  pull_request:
  workflow_dispatch:
jobs:
  dm-reverb:
    strategy:
      fail-fast: false
      matrix:
        os: [ ubuntu-24.04, ubuntu-24.04-arm ]  # ARM always fails!!
        python-version: [ "3.8", "3.9", "3.10", "3.11" ]
    runs-on: ${{ matrix.os }}
    steps:
    - uses: actions/setup-python@v5
      with:
        python-version: ${{ matrix.python-version }}
    - run: pip install dm-reverb

image

If someone can add ruff to Pipfile.lock then I can rebase this PR getting rid of the pipx trick.

boomanaiden154 added a commit that referenced this pull request Jan 29, 2025
Similar to 8cb41b8. A couple new fixes
now that more code has been landed before the CI check went up.
Separating this out from #421 to try and keep the commit history
somewhat clean.
@boomanaiden154
Copy link
Collaborator

If someone can add ruff to Pipfile.lock then I can rebase this PR getting rid of the pipx trick.

Done. Will land once CI passes.

@boomanaiden154 boomanaiden154 merged commit 4893898 into google:main Jan 29, 2025
13 checks passed
@cclauss cclauss deleted the lint-with-ruff branch January 29, 2025 08:25
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