Skip to content

Conversation

@simone99n
Copy link
Contributor

Description

In this DRAFT PR, we remove the use of getattr() from several places:

  • tokenizer_masking.py
    The use of getattr() was unnecessary, since self.rng = None is explicitly defined in __init__().
  • loss_calculator.py and loss_module_physical.py
    We now rely on __dict__ instead of getattr(). However, this approach is still not safe.
    • A possible improvement would be to introduce a REGISTRY for both loss_fns and LossModules.
  • test_cli.py
  • better_abc.py

Additionally, in pyproject.toml, I attempted to enable automatic detection of getattr() usage, but it is currently not working as expected.

Issue Number

Checklist before asking for review

  • I have performed a self-review of my code
  • My changes comply with basic sanity checks:
    • I have fixed formatting issues with ./scripts/actions.sh lint
    • I have run unit tests with ./scripts/actions.sh unit-test
    • I have documented my code and I have updated the docstrings.
    • I have added unit tests, if relevant
  • I have tried my changes with data and code:
    • I have run the integration tests with ./scripts/actions.sh integration-test
    • (bigger changes) I have run a full training and I have written in the comment the run_id(s): launch-slurm.py --time 60
    • (bigger changes and experiments) I have shared a hegdedoc in the github issue with all the configurations and runs for this experiments
  • I have informed and aligned with people impacted by my change:
    • for config changes: the MatterMost channels and/or a design doc
    • for changes of dependencies: the MatterMost software development channel

@clessig
Copy link
Collaborator

clessig commented Jan 28, 2026

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

@simone99n
Copy link
Contributor Author

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

Did you try also to run ./scripts/actions.sh type-check locally?

The Github workflow for the linting is the following:

  1. ./scripts/actions.sh lint-check
  2. ./scripts/actions.sh toml-check
  3. ./scripts/actions.sh type-check

@clessig
Copy link
Collaborator

clessig commented Jan 29, 2026

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

Did you try also to run ./scripts/actions.sh type-check locally?

The Github workflow for the linting is the following:

  1. ./scripts/actions.sh lint-check
  2. ./scripts/actions.sh toml-check
  3. ./scripts/actions.sh type-check

And what is run with ./scripts/actions.sh lint. Does this miss any of the three steps?

@simone99n
Copy link
Contributor Author

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

Did you try also to run ./scripts/actions.sh type-check locally?
The Github workflow for the linting is the following:

  1. ./scripts/actions.sh lint-check
  2. ./scripts/actions.sh toml-check
  3. ./scripts/actions.sh type-check

And what is run with ./scripts/actions.sh lint. Does this miss any of the three steps?

  • ./scripts/actions.sh lint --> ruff formatting
  • ./scripts/actions.sh lint-check --> ruff formatting check (only check!) + pylint formatting

I may be missing something, but I don’t understand why we have two different linting approaches.
The PR template encourages developers to run ./scripts/actions.sh lint before pushing code, while the GitHub PR checks run ./scripts/actions.sh lint-check + ./scripts/actions.sh toml-check + ./scripts/actions.sh type-check

@clessig
Copy link
Collaborator

clessig commented Feb 1, 2026

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

Did you try also to run ./scripts/actions.sh type-check locally?
The Github workflow for the linting is the following:

  1. ./scripts/actions.sh lint-check
  2. ./scripts/actions.sh toml-check
  3. ./scripts/actions.sh type-check

And what is run with ./scripts/actions.sh lint. Does this miss any of the three steps?

  • ./scripts/actions.sh lint --> ruff formatting
  • ./scripts/actions.sh lint-check --> ruff formatting check (only check!) + pylint formatting

I may be missing something, but I don’t understand why we have two different linting approaches. The PR template encourages developers to run ./scripts/actions.sh lint before pushing code, while the GitHub PR checks run ./scripts/actions.sh lint-check + ./scripts/actions.sh toml-check + ./scripts/actions.sh type-check

I think ./scripts/actions.sh lint should be a short-cut for the three separate checks that are run on Github. Is there anything speaking against this?

@simone99n
Copy link
Contributor Author

@simone99n : related issue but maybe you have some insights since you are working on this: we currently have a gap between linting locally (using scripts/actions.sh lint) and what happens on github. Specifically, if class members are not initialized in the ctor then github linting throws an error but local linting does not. Any idea where this comes from and how to fix it?

Did you try also to run ./scripts/actions.sh type-check locally?
The Github workflow for the linting is the following:

  1. ./scripts/actions.sh lint-check
  2. ./scripts/actions.sh toml-check
  3. ./scripts/actions.sh type-check

And what is run with ./scripts/actions.sh lint. Does this miss any of the three steps?

  • ./scripts/actions.sh lint --> ruff formatting
  • ./scripts/actions.sh lint-check --> ruff formatting check (only check!) + pylint formatting

I may be missing something, but I don’t understand why we have two different linting approaches. The PR template encourages developers to run ./scripts/actions.sh lint before pushing code, while the GitHub PR checks run ./scripts/actions.sh lint-check + ./scripts/actions.sh toml-check + ./scripts/actions.sh type-check

I think ./scripts/actions.sh lint should be a short-cut for the three separate checks that are run on Github. Is there anything speaking against this?

It makes sense to me 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Lint - disable using getattr in the project

2 participants