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 linter/formatter #15

Merged
merged 16 commits into from
Nov 24, 2023
Merged

Add Ruff linter/formatter #15

merged 16 commits into from
Nov 24, 2023

Conversation

evanderiel
Copy link
Collaborator

Adds Ruff configuration and does initial reformatting for all files.

If you want to run Ruff on the codebase, it's

ruff check aana

You can automatically fix some issues with the --fix and --unsafe-fixes options. (Be sure to install the dev dependencies: poetry install --with=dev. )

To run the auto-formatter, it's

ruff format aana

If you want to enable this as a local pre-commit hook, additionally run the following:

git config core.hooksPath .githooks

You can also simply run .githooks/pre-commit manually if you prefer.

A Github Action for the same thing to run as a hook isn't present yet due to problems I've run into getting poetry to work correctly.

@HRashidi
Copy link
Contributor

Currently I got a error related to the poetry lock. Do we need to update the lock on the repo each times (as you added the rufff) or we can add it to gitignore!>

@HRashidi
Copy link
Contributor

ruff throw errors for using assert inside the utils. Should it be deactivated or maybe change the the assert to use the value_error instead

@movchan74
Copy link
Contributor

movchan74 commented Nov 21, 2023

I suggest we add ruff extension as recommendations to .vscode/extensions.json and add it to the devcontainer.json.

Copy link
Contributor

@movchan74 movchan74 left a comment

Choose a reason for hiding this comment

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

Looks good. I added few suggestions.

And I think these issues need to be fixed before merge:

  1. What you mentioned in the PR description should be added to the README.
  2. devcontainer needs to run poetry install with --with=dev. Maybe add pre-commit hooks too.
  3. There is one issue reported by ruff: aana/tests/utils.py:32:5: S101 Use of assert detected

aana/deployments/whisper_deployment.py Outdated Show resolved Hide resolved
aana/models/pydantic/sampling_params.py Outdated Show resolved Hide resolved
aana/models/pydantic/whisper_params.py Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
@evanderiel
Copy link
Collaborator Author

Currently I got a error related to the poetry lock. Do we need to update the lock on the repo each times (as you added the rufff) or we can add it to gitignore!>

Poetry guidelines say to include the lock file in source control, but it seems like it's causing more trouble than it's worth at the moment. @movchan74 what do you think? Should we .gitignore it for now until we can figure out what's making it cause problems?

@movchan74
Copy link
Contributor

Currently I got a error related to the poetry lock. Do we need to update the lock on the repo each times (as you added the rufff) or we can add it to gitignore!>

Poetry guidelines say to include the lock file in source control, but it seems like it's causing more trouble than it's worth at the moment. @movchan74 what do you think? Should we .gitignore it for now until we can figure out what's making it cause problems?

I'm not sure. Sometimes it helps, sometimes it creates problems. For example, it was useful when onnx-runtime updated the package but didn't release all wheels so poetry install was failing. So it is useful for reproducibility, but we have to make sure we keep it up-to-date. I would keep it.

@evanderiel
Copy link
Collaborator Author

  1. What you mentioned in the PR description should be added to the README.
    Done.
  1. devcontainer needs to run poetry install with --with=dev. Maybe add pre-commit hooks too.

Can I just add this to install.sh? Or does it need to done differently? I've never used dev containers.

  1. There is one issue reported by ruff: aana/tests/utils.py:32:5: S101 Use of assert detected

Fixed.

@movchan74
Copy link
Contributor

  1. What you mentioned in the PR description should be added to the README.
    Done.
  1. devcontainer needs to run poetry install with --with=dev. Maybe add pre-commit hooks too.

Can I just add this to install.sh? Or does it need to done differently? I've never used dev containers.

  1. There is one issue reported by ruff: aana/tests/utils.py:32:5: S101 Use of assert detected

Fixed.

install.sh is also used in Dockerfile for a "production" image. So we probably need a flag for install.sh to set it to dev.

@evanderiel evanderiel merged commit ac309e8 into main Nov 24, 2023
1 check passed
@evanderiel evanderiel deleted the edr/ruff branch November 24, 2023 14:16
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