Skip to content

Conversation

@nkakouros
Copy link
Contributor

With the changes happening in #87, types are a bit out of sync in places. This adds mypy in strict mode as a CI job. Ideally, this mypy workflow and the ruff one would exist only in the mal-toolbox repo and be used here as reusable workflows. This will give a common static analysis coverage on both projects (and maybe others too).

@nkakouros
Copy link
Contributor Author

These are a lot of errors...

@mrkickling
Copy link
Contributor

Currently 160 errors in mypy, we need to fix those before merging.

@mrkickling
Copy link
Contributor

I fixed most of the errors in malsim, but I realized I missed ./tests. This PR is work in progress still so I will put it into Draft.

@mrkickling mrkickling marked this pull request as draft June 2, 2025 12:49
@mrkickling
Copy link
Contributor

mrkickling commented Jun 2, 2025

Now all CIs are passing.

I had to do a few things:

  • Add typehints to everything in the code base (I decided to ignore the gym_envs which I don't understand well enough, believe me I tried)
  • Ignore missing imports since maltoolbox does not provide a required file to show that is supports mypy (maybe you know better how that works)
  • Change so the CI install scripts used .[ml] and .[dev] instead of .[all] (since that was not defined)

I think this is mergeable. It does give mypy support for most of the code base. We should create an issue in the maltoolbox repo so we don't have to use 'ignore missing imports'. We should also typehint (or deprecate) the gym_envs.

nkakouros and others added 9 commits June 3, 2025 10:21
- Add typehints for all files except for gym_envs.py
- mypy runs without errors
- remove the MalSimEnv baseclass for malsim vectorized obs env
- remove logging methods from malsim vectorized obs env as cleanup
@mrkickling mrkickling marked this pull request as ready for review June 3, 2025 08:26
@mrkickling mrkickling merged commit 7b4c4c2 into main Jun 26, 2025
7 checks passed
@mrkickling mrkickling deleted the mypy branch June 26, 2025 12:31
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