-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
Integrate and Enhance Type Checking with Mypy #264
Conversation
… subset of files, update requirements.txt to reflect mypy version, as well as fix typing issues in instructor/cli/usage.py.
WalkthroughThe project has introduced a new continuous integration workflow, enhanced type checking with mypy, and updated the pre-commit configuration. Type aliases and hints have been refined across various Python files, and dependency changes reflect a shift towards mypy and typer, moving away from ruff. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- poetry.lock
- pyproject.toml
Files selected for processing (7)
- .github/workflows/mypy.yml (1 hunks)
- .mypy.ini (1 hunks)
- .pre-commit-config.yaml (1 hunks)
- instructor/_types/_alias.py (1 hunks)
- instructor/cli/usage.py (7 hunks)
- instructor/sandbox.py (1 hunks)
- requirements.txt (1 hunks)
Files skipped from review due to trivial changes (1)
- instructor/_types/_alias.py
Additional comments: 3
.mypy.ini (1)
- 1-29: The
.mypy.ini
configuration aligns with the PR's objectives to enhance type checking capabilities and code quality. The settings are strict and closely follow the standards used by OpenAI's Python projects, as mentioned in the PR objectives. The references to other projects'mypy
configurations are a good practice for maintaining consistency and standards.instructor/sandbox.py (1)
- 1-3: The code appears to be a simple comparison operation without any context of usage or integration with the rest of the system. Without additional context, it's difficult to assess the relevance of this change to the PR's objectives. However, the code itself is correct and does not exhibit any issues.
requirements.txt (1)
- 6-9: The changes to
requirements.txt
align with the PR's objective to integratemypy
for static type checking and improve code quality. The addition ofmypy==1.7.1
andtyper
, and the removal ofruff==0.1.7
are consistent with the stated goals.
hooks: | ||
- id: ci_type_mypy | ||
name: Static Type Check | ||
entry: > | ||
bash -c 'set -o pipefail; | ||
export CUSTOM_PACKAGES="instructor/cli/cli.py instructor/cli/usage.py" && | ||
export CUSTOM_FLAGS="--python-version=3.9 --color-output --no-pretty" && | ||
curl -sSL https://github.com/gao-hongnan/omniverse/continuous-integration/scripts/devops/continuous-integration/type_mypy.sh | | ||
bash' | ||
language: system | ||
types: [python] | ||
pass_filenames: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new ci_type_mypy
hook is correctly configured to perform static type checks using mypy. The entry script is fetched from a remote repository, which could be a potential security risk if the script content changes. Consider pinning the script to a specific commit hash to mitigate this risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Are you planning to add more before merge?
…from continuous-integration to MyPy for better readability.
If this PR looks decent to you, I can help set up a more complete suite of pre-merge continuous integration (CI) checks. |
would love that! |
Got it, I will open a new PR for that. For now, I am finding out why the latest |
Hi @jxnl, after inspecting the errors arising from the pre-merge test, it seems that all failed cases have exactly the same error: openai.APIConnectionError: Connection error. which may be related to the secrets In light of this error, it could be because GitHub Actions provides restricted access to secrets when running workflows triggered by pull requests from forks. This is further confirmed after I checked stackoverflow and a blog post from GitHub. The blog post mentioned it can be somewhat (unsure) resolved by adding |
that makes sense running tests now |
You might have to first define the following in pull_request_target:
branches:
- main and make a push to the main branch first. But after reading a bunch of articles, there are still potential risks involved, see here. As of now I do not have a better alternative given that the recent changes also depends on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
…ion to mypy to be in sync with the naming convention of other workflows jxnl#264.
This PR introduces several enhancements and changes focusing on integrating and utilizing
mypy
for improved type checking and code quality. Below are the key points of this PR:✅ Mypy Configuration File: Closely aligned with the OpenAI Python mypy settings.
✅ Pin Mypy Versions: The versions of
mypy
are pinned.✅ Mypy Pre-commit Implementation: Integrated
mypy
into the pre-commit hooks.✅ Mypy GitHub Actions: Implemented GitHub Actions for running
mypy
checks.✅ Mypy Artifacts Logs for Persistency: Configured the workflow to generate and store
mypy
logs as artifacts. This provides a persistent record of the type checks for each build, useful for debugging and historical reference.✅ Fixed Mypy Errors in
cli/cli.py
andcli/usage.py
: As a proof of concept (POC), type errors incli/cli.py
andcli/usage.py
have been fixed. Other errors can be fixed gradually as a collective effort.Some screenshots.
As future enhancement, we can group
ruff
,mypy
,test
etc under a single workflow in github actions.Summary by CodeRabbit
New Features
ModelNames
for improved type checking and clarity in code.Documentation
Refactor
ruff-format
hook from the pre-commit configuration.Dependencies
mypy
version1.7.1
to dependencies for static type checking.typer
in the list of dependencies for creating command-line interfaces.ruff
version0.1.7
from dependencies.Bug Fixes