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

Update pre-commit dependencies #353

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

altheaden
Copy link
Contributor

@altheaden altheaden commented Oct 2, 2024

This PR updates the dependencies within .pre-commit-config.yaml to their most recent versions. It also adds pre-commit CI which will hopefully auto-update these dependencies on a monthly basis.


Updated template from @forsyth2:

Issue resolution

This pull request is a minor adjustment that does not affect functional code, aside from minor renamings.

1. Does this do what we want it to do?

Objectives:

  • Update the dependencies within .pre-commit-config.yaml to their most recent versions.
  • Include the pre-commit type changes/fixes introduced by this.
  • Add pre-commit CI which will hopefully auto-update these dependencies on a monthly basis.

Required:

  • Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
  • Testing: I have added at least one automated test. Every objective above is represented in at least one test.
    • Changes to functional code are minimal.
  • Testing: I have considered likely and/or severe edge cases and have included them in testing.

2. Are the implementation details accurate & efficient?

Required:

  • Logic: I have visually inspected the entire pull request myself.

3. Is this well documented?

Required:

  • Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.
    • This change does not require new documentation.

4. Is this code clean?

Required:

  • Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
  • Pre-commit checks: All the pre-commits checks have passed.

@altheaden
Copy link
Contributor Author

altheaden commented Oct 3, 2024

@forsyth2 Following the discussion in E3SM-Project/zppy#627 I started making pre-commit changes here. The flake8 fixes are straightforward but mypy is having issues that I'm not sure how to fix:

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
black....................................................................Passed
isort....................................................................Passed
flake8...................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

zstash/globus.py:13: error: Library stubs not installed for "six.moves.urllib.parse"  [import-untyped]
zstash/hpss.py:7: error: Library stubs not installed for "six.moves.urllib.parse"  [import-untyped]
zstash/hpss.py:7: note: Hint: "python3 -m pip install types-six"
zstash/hpss.py:7: note: (or run "mypy --install-types" to install all missing stub packages)
zstash/hpss.py:7: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
zstash/hpss_utils.py:180: error: Unused "type: ignore" comment  [unused-ignore]
zstash/hpss_utils.py:219: error: Unused "type: ignore" comment  [unused-ignore]
zstash/create.py:11: error: Library stubs not installed for "six.moves.urllib.parse"  [import-untyped]
zstash/extract.py:353: error: Name "worker_idx" already defined on line 340  [no-redef]
zstash/extract.py:448: error: Unused "type: ignore" comment  [unused-ignore]
zstash/extract.py:543: error: Unused "type: ignore" comment  [unused-ignore]
Found 8 errors in 5 files (checked 29 source files)

I believe I've setup the dev environment appropriately (although I could have messed that up), but I am not sure how to approach fixing this. I can keep looking into it but I figured I would update here in case it's just something I'm doing wrong with my setup.

(Some of these errors look easy to fix, I am mostly referring to the importing errors)

@forsyth2
Copy link
Collaborator

forsyth2 commented Oct 3, 2024

Hmm I don't think I've seen errors like this before.

hooks:
- id: mypy
args: ["--config=setup.cfg"]
args: ["--config=setup.cfg", "--install-types", "--non-interactive"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these two arguments which auto-fix the importing errors which were apparently added in a previous mypy update as discussed here: https://www.github.com/python/mypy/issues/10632#issuecomment-863332698

@altheaden altheaden force-pushed the update-pre-commit-deps branch 5 times, most recently from a26a08d to c064b25 Compare October 14, 2024 18:04
@@ -350,7 +350,7 @@ def multiprocess_extract(
workers_to_matches: List[List[FilesRow]] = [[] for _ in range(num_workers)]
for db_row in matches:
tar = db_row.tar
workers_idx: int
worker_idx: int
Copy link
Contributor Author

@altheaden altheaden Oct 14, 2024

Choose a reason for hiding this comment

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

This rename was to match the index variable in the subsequent for loop, but it was obscuring a different error (redundant variable definition) which mypy is picking up on now. I'll leave this for somebody who knows the code better to sort out, but it seems like a pretty easy fix.

Copy link
Collaborator

@forsyth2 forsyth2 Oct 14, 2024

Choose a reason for hiding this comment

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

Ah ok so, the whole relevant block on the main branch is

    # Using a greedy approach, populate workers_to_tars.
    for _, tar in enumerate(tar_to_size):
        # The worker with the least work should get the current largest amount of work.
        workers_work: int
        worker_idx: int
        workers_work, worker_idx = heapq.heappop(work_to_workers)
        workers_to_tars[worker_idx].add(tar)
        # Add this worker back to the heap, with the new amount of work.
        worker_tuple: Tuple[float, int] = (workers_work + tar_to_size[tar], worker_idx)
        # FIXME: error: Cannot infer type argument 1 of "heappush"
        heapq.heappush(work_to_workers, worker_tuple)  # type: ignore

    # For worker i, workers_to_matches[i] is a list of
    # matches from the database for it to process.
    workers_to_matches: List[List[FilesRow]] = [[] for _ in range(num_workers)]
    for db_row in matches:
        tar = db_row.tar
        workers_idx: int
        for worker_idx in range(len(workers_to_tars)):
            if tar in workers_to_tars[worker_idx]:
                # This worker gets this db_row.
                workers_to_matches[worker_idx].append(db_row)

So yeah once workers_idx: int is changed to worker_idx: int to actually match the variable in this loop, mypy complains because we're attempting to re-define a variable from the previous loop. It looks like in practice that doesn't actually matter since each occurrence of worker_idx stays inside its scoping (i.e. its loop), but mypy doesn't have a way to know that.

I think here I'd say to actually keep it defined as workers_idx: int and change that one loop accordingly:

        workers_idx: int
        for workers_idx in range(len(workers_to_tars)):
            if tar in workers_to_tars[workers_idx]:
                # This worker gets this db_row.
                workers_to_matches[workers_idx].append(db_row)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will do that!

@altheaden
Copy link
Contributor Author

@forsyth2 This branch is sorted out to the extent that I feel comfortable changing the code, but there is one remaining mypy error which I've pointed out above. It looks like a relatively easy fix would be to rename one of the index variables, but I wanted to leave that change for somebody else to make sure it gets fixed correctly.

I fixed the importing errors mypy was complaining about (see my first comment above) but the fix could impact things if it's automatically finding the wrong library stubs.

Please let me know if there is anything else I can help with, or if I should change anything here.

@@ -216,7 +216,7 @@ def add_file(
blocks += 1
# Increase the offset by the amount already saved to the tar
# FIXME: error: "TarFile" has no attribute "offset"
tar.offset += blocks * tarfile.BLOCKSIZE # type: ignore
tar.offset += blocks * tarfile.BLOCKSIZE
Copy link
Collaborator

@forsyth2 forsyth2 Oct 14, 2024

Choose a reason for hiding this comment

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

If these lines are passing the mypy check now, we can probably remove all the error comments on the lines directly above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll take those out right now.

@altheaden altheaden force-pushed the update-pre-commit-deps branch from 962b8a2 to 8f7fa35 Compare October 14, 2024 21:14
@altheaden
Copy link
Contributor Author

@forsyth2 Those changes should be sorted now, looks like everything is passing now as expected.

@forsyth2 forsyth2 merged commit 082afc7 into E3SM-Project:main Oct 14, 2024
3 checks passed
@forsyth2
Copy link
Collaborator

@altheaden Ok it looks like I've now merged in all your open PRs (E3SM-Project/zppy#620 / E3SM-Project/zppy#630, E3SM-Project/zppy#621, E3SM-Project/zppy#627 in zppy and #351, #352, #353 in zstash). Thanks so much for working on these!

@altheaden altheaden deleted the update-pre-commit-deps branch October 15, 2024 15:14
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.

2 participants