Skip to content

Conversation

AlexanderViand-Intel
Copy link
Contributor

@AlexanderViand-Intel AlexanderViand-Intel commented Jul 29, 2025

This PR consists of three parts:

  1. Improvements to the pre-commit setup

    • pre-commit now runs hermetically (does not require local env to be set up)
    • switched to the significantly faster (and hermetically runnable) ruff over pylint (also subsumes black formatting)
    • Various other small fixes, such as fixing BOM characters that were messing with license header insertion
    • Adds a pyproject.toml that serves as a single source of truth for both pre-commit formatting/linting as well as editors/IDEs
  2. A commit recording the state of the repo after running pre-commit run --all-files (i.e., all automatic changes)

  3. Manual fixes that were required to get pre-commuit run --all-files to finally pass completely.
    Note that this is just fixing the tip of the iceberg: the setup includes lots of general global exceptions to the such as relaxed naming/casing rules, and nearly the whole assembler being excepted, in order to avoid needing to change vast numbers of variable/function/class names.

Since the second commit introduces a lot of diff "noise", I suggest using the commit filter during review:

@AlexanderViand-Intel
Copy link
Contributor Author

AlexanderViand-Intel commented Jul 30, 2025

The changes broke some of the tests (all fixed now ✅), specifically :

  • the linter removes redundant (same as default value) arguments from calls and that tripped up pydantic and some expect_calls
  • the linter strictly prefers isinstance(x, A | B | C) which broke a complex mocking setup used in the assembler tests. Instead of making the mocking even more complex, I switched to using MockMagic's spec feature so that the mock objects work with unmodified isinstance.

@joserochh I think you were the last one to touch that mocking setup, so I'd especially value your feedback on these changes

@faberga
Copy link
Collaborator

faberga commented Jul 30, 2025

Hi @AlexanderViand, thanks for splitting the PRs.

Can we hold this PR to go after PR #104 ? The reason being that PR 104 updates and changes the program mapper and functional modeler README, plus file doc locations, and bring in trace files for tutorials.

Thanks

Copy link
Collaborator

@kylanerace kylanerace left a comment

Choose a reason for hiding this comment

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

Is there a particular reason the linter prefers isinstance(x, A | B | C) (mentioned here) as opposed to isinstance(x, (A, B, C))? If I recall, this is generally a Python 3.10+ feature? Seems like it would decrease version compatibility mainly for formatting preference, or is there a separate benefit?

@AlexanderViand-Intel
Copy link
Contributor Author

AlexanderViand-Intel commented Jul 30, 2025

Is there a particular reason the linter prefers isinstance(x, A | B | C) (mentioned here) as opposed to isinstance(x, (A, B, C))?

Good question! I just looked up which specific rule is responsible for it (it's UP308) and it's actually slated to be deprecated already, so I think we can just ignore it globally for now! I'll push a commit to revert the changes :)

If I recall, this is generally a Python 3.10+ feature? Seems like it would decrease version compatibility mainly for formatting preference, or is there a separate benefit?

I don't think we need to worry about 3.9 support anymore - it's about to hit EOL, and Ubuntu has shipped a 3.10+ version for the last two LTS, so you'd have to be on a really outdated system for this to matter. The reason the rule was deprecated doesn't seem to be about 3.9 compatibility, but because this syntax produces slightly slower code than the old one?

@joserochh
Copy link
Contributor

Thank you @AlexanderViand-Intel, I like this formatting better. And the automatic fixes were great. The only thing I see is the stacked license notes for both 2025 and 2024.

@AlexanderViand-Intel
Copy link
Contributor Author

AlexanderViand-Intel commented Jul 30, 2025

Thank you @AlexanderViand-Intel, I like this formatting better. And the automatic fixes were great. The only thing I see is the stacked license notes for both 2025 and 2024.

I think this is because of how the license insertion was set-up, where it would only recognize the exact current license header string. After looking into it, it looks like the hook can be configured to use fuzzy matching, which I've now set up (and fixed the existing duplicates)

@joserochh
Copy link
Contributor

Thank you @AlexanderViand-Intel, I like this formatting better. And the automatic fixes were great. The only thing I see is the stacked license notes for both 2025 and 2024.

I think this is because of how the license insertion was set-up, where it would only recognize the exact current license header string. After looking into it, it looks like the hook can be configured to use fuzzy matching, which I've now set up (and fixed the existing duplicates)

There are still ~16 files with duplicated license. I know we should leave one note, but @kylanerace is trying to get clarification on the dates we need to have.

@kylanerace
Copy link
Collaborator

kylanerace commented Aug 4, 2025

Thank you @AlexanderViand-Intel, I like this formatting better. And the automatic fixes were great. The only thing I see is the stacked license notes for both 2025 and 2024.

I think this is because of how the license insertion was set-up, where it would only recognize the exact current license header string. After looking into it, it looks like the hook can be configured to use fuzzy matching, which I've now set up (and fixed the existing duplicates)

There are still ~16 files with duplicated license. I know we should leave one note, but @kylanerace is trying to get clarification on the dates we need to have.

OK, the internal guidance is to do a range from the first license date of the file creation to now e.g. for a file licensed/created in 2024:

// Copyright (C) 2024 - 2025 Intel Corporation
// SPDX-License-Identifier: Apache-2.0

@AlexanderViand
Copy link
Contributor

OK, the internal guidance is to do a range from the first license date of the file creation to now e.g. for a file licensed/created in 2024:

// Copyright (C) 2024 - 2025 Intel Corporation
// SPDX-License-Identifier: Apache-2.0

I don't think the pre-commit hook we use (or any, for that matter) can achieve that pattern, but what I was able to do just now is set the hook to allow prior years (this avoides the duplicates @joserochh noticed) and allows each file to have the date be its creation year. I think without writing a custom hook, this is probably as close as we can get :)

joserochh
joserochh previously approved these changes Aug 4, 2025
Copy link
Contributor

@joserochh joserochh left a comment

Choose a reason for hiding this comment

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

LGTM

@kylanerace
Copy link
Collaborator

OK, the internal guidance is to do a range from the first license date of the file creation to now e.g. for a file licensed/created in 2024:
// Copyright (C) 2024 - 2025 Intel Corporation
// SPDX-License-Identifier: Apache-2.0

I don't think the pre-commit hook we use (or any, for that matter) can achieve that pattern, but what I was able to do just now is set the hook to allow prior years (this avoides the duplicates @joserochh noticed) and allows each file to have the date be its creation year. I think without writing a custom hook, this is probably as close as we can get :)

Yeah this sounds good. Otherwise It'll probably be non-tenable to maintain

kylanerace
kylanerace previously approved these changes Aug 4, 2025
Copy link
Collaborator

@kylanerace kylanerace left a comment

Choose a reason for hiding this comment

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

LGTM, should be fine to merge pending #104 merging first, following @faberga's request

AlexanderViand and others added 9 commits August 6, 2025 16:35
* updated tests to expect new calls (w/o redundant default arguments passed in)
* made pydantic happy by explicitly defaulting an int | None arg to None
* Mocking for instruction in assembler tests is now done with `spec` instead of overwriting `isinstance`
@AlexanderViand
Copy link
Contributor

LGTM, should be fine to merge pending #104 merging first, following @faberga's request

With #104 now merged, I've rebased this PR onto main (there was a conflict before), should be good to go now.

@kylanerace kylanerace self-requested a review August 6, 2025 23:47
Copy link
Collaborator

@kylanerace kylanerace left a comment

Choose a reason for hiding this comment

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

LGTM, approved prior, now rebased. Should be good to go

@kylanerace kylanerace merged commit f334b11 into main Aug 7, 2025
8 checks passed
@kylanerace kylanerace deleted the aviand/pre-commit branch August 7, 2025 02:33
@faberga
Copy link
Collaborator

faberga commented Aug 8, 2025

@AlexanderViand-Intel, thanks for getting this PR in. Looking forward to PR #111.

@AlexanderViand-Intel
Copy link
Contributor Author

@AlexanderViand-Intel, thanks for getting this PR in. Looking forward to PR #111.

#111 should be ready to review :)

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.

5 participants