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

Fix 199 ruff errors #689

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Fix 199 ruff errors #689

merged 8 commits into from
Sep 9, 2024

Conversation

hoh
Copy link
Member

@hoh hoh commented Sep 3, 2024

Explain what problem this PR is resolving

The python linter ruff is not enable on this project because it raises too many errors to fix at once. At the time of writing, ruff check src raises 375 errors.

Self proofreading checklist

Not relevant for these changes.

  • The new code clear, easy to read and well commented.
  • New code does not duplicate the functions of builtin or popular libraries.
  • An LLM was used to review the new code and look for simplifications.
  • New classes and functions contain docstrings explaining what they provide.
  • All new code is covered by relevant tests.
  • Documentation has been updated regarding these changes.

Changes

The first commit contains the result of ruff check src --fix, which contains safe automatic fixes to the codebase.

I then ran ruff check src --fix --unsafe-fixes and added the following commits with the changes that I reviewed manually and deem ready to commit:

  • Fix: Generator loop -> 'yield from'
  • Fix: Type annotations could be improved
  • Fix: Python < 3.10 is no supported anymore
  • Fix: Ruff errored on exception raising

How to test

Run ruff check src or hatch run testing:ruff.

ruff check src on the code found 176 errors after the changes.

Copy link
Collaborator

@Psycojoker Psycojoker left a comment

Choose a reason for hiding this comment

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

I've reviewed every commits up one by one and all seems to make sens (even if I don't get the point of extracting a message from the exception?) and everything seems good 👍

On the other hand I don't understand why you haven't added ruff to the CI? Also the CI is failing weirdly for things unrelated to this PR I think?

@hoh
Copy link
Member Author

hoh commented Sep 3, 2024

I had the same reflection: Not sure why the CI failed on this PR while it passed for another one around the same time... 🤔

We did not add ruff to the CI because the remaining 176 errors contain non trivial changes and I don't know of any tool similar to codecov but that would look at the increment/decrement in linter errors in the codebase. Seemed like all or nothing.

@olethanh
Copy link
Collaborator

olethanh commented Sep 4, 2024

it seem ruff and black don't agree on some formatting.

Syncing dependencies
cmd [1] | black --check --diff .
--- /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/conf.py	2024-09-03 16:53:16.680400+00:00
+++ /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/conf.py	2024-09-03 16:55:05.973700+00:00
@@ -284,13 +284,11 @@
would reformat /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/conf.py
     BENCHMARK_FAKE_DATA_PROGRAM = Path(abspath(join(__file__, "../../../../examples/example_fastapi")))
 
     FAKE_DATA_MESSAGE = Path(abspath(join(__file__, "../../../../examples/program_message_from_aleph.json")))
     FAKE_DATA_DATA: Path | None = Path(abspath(join(__file__, "../../../../examples/data/")))
     FAKE_DATA_RUNTIME = Path(abspath(join(__file__, "../../../../runtimes/aleph-debian-12-python/rootfs.squashfs")))
-    FAKE_DATA_VOLUME: Path | None = Path(
-        abspath(join(__file__, "../../../../examples/volumes/volume-venv.squashfs"))
-    )
+    FAKE_DATA_VOLUME: Path | None = Path(abspath(join(__file__, "../../../../examples/volumes/volume-venv.squashfs")))
 
     # Tests on instances
 
     TEST_INSTANCE_ID: str | None = Field(
         default=None,  # TODO: Use a valid item_hash here
--- /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/models.py	2024-09-03 16:53:16.680400+00:00
+++ /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/models.py	2024-09-03 16:55:06.231799+00:00
@@ -184,11 +184,13 @@
would reformat /home/runner/work/aleph-vm/aleph-vm/src/aleph/vm/models.py
             if self.resources:
                 # Already prepared
                 return
 
             self.times.preparing_at = datetime.now(tz=timezone.utc)
-            resources: AlephProgramResources | AlephInstanceResources | AlephQemuResources | AlephQemuConfidentialInstance
+            resources: (
+                AlephProgramResources | AlephInstanceResources | AlephQemuResources | AlephQemuConfidentialInstance
+            )
             if self.is_program:
                 resources = AlephProgramResources(self.message, namespace=self.vm_hash)
             elif self.is_instance:
                 if self.hypervisor == HypervisorType.firecracker:
                     resources = AlephInstanceResources(self.message, namespace=self.vm_hash)

I have also checked running hatch fmt which also run ruff but I think under another config and it make other changes. Such as

-async def get_message(hash_: str) -> Optional[dict]:
+async def get_message(hash_: str) -> dict | None:

edit: just noticed they are in the test directory

@olethanh
Copy link
Collaborator

olethanh commented Sep 4, 2024

I have pushed more ruff fix and a black run

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 63.76147% with 79 lines in your changes missing coverage. Please review.

Project coverage is 62.32%. Comparing base (fbbbf3d) to head (24c6594).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/aleph/vm/orchestrator/payment.py 0.00% 16 Missing ⚠️
src/aleph/vm/models.py 68.00% 8 Missing ⚠️
src/aleph/vm/utils/__init__.py 38.46% 8 Missing ⚠️
src/aleph/vm/controllers/qemu/instance.py 53.84% 6 Missing ⚠️
src/aleph/vm/network/interfaces.py 40.00% 6 Missing ⚠️
src/aleph/vm/orchestrator/chain.py 44.44% 5 Missing ⚠️
src/aleph/vm/orchestrator/views/authentication.py 44.44% 5 Missing ⚠️
...c/aleph/vm/hypervisors/qemu_confidential/qemuvm.py 0.00% 4 Missing ⚠️
src/aleph/vm/orchestrator/cli.py 0.00% 4 Missing ⚠️
src/aleph/vm/guest_api/__main__.py 25.00% 3 Missing ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
- Coverage   62.69%   62.32%   -0.38%     
==========================================
  Files          69       69              
  Lines        6069     6073       +4     
  Branches      642      638       -4     
==========================================
- Hits         3805     3785      -20     
- Misses       2113     2139      +26     
+ Partials      151      149       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoh hoh merged commit acc2302 into main Sep 9, 2024
21 of 22 checks passed
@hoh hoh deleted the hoh-fix-ruff-warnings branch September 9, 2024 13:01
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