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

chore: move python integ tests to uv #1424

Merged
merged 12 commits into from
Feb 7, 2025
Merged

chore: move python integ tests to uv #1424

merged 12 commits into from
Feb 7, 2025

Conversation

sxlijin
Copy link
Collaborator

@sxlijin sxlijin commented Feb 7, 2025

New commands to run python integ tests:

# from within integ-tests/python
uv run maturin develop --uv --manifest-path ../../engine/language_client_python/Cargo.toml"
uv run baml-cli generate --from ..
infisical run --env=test -- uv run pytest"

References to poetry have been updated to point to uv.

Also delete old github workflows that haven't been relevant for almost a year.


Important

Transition Python integration tests from Poetry to uv, update related scripts and documentation, and remove outdated GitHub workflows.

  • Behavior:
    • Transition Python integration tests from Poetry to uv in integ-tests/python.
    • Update test execution commands in run_tests.sh and integ-tests/run-tests.sh to use uv.
    • Remove outdated GitHub workflows related to old testing setups.
  • Dependencies:
    • Update pyproject.toml to reflect uv usage and remove Poetry references.
    • Add uv.lock for dependency management.
  • Documentation:
    • Update README.md files in integ-tests and integ-tests/python to guide using uv for setup and testing.
  • Misc:
    • Remove tools/install script as it is no longer relevant.

This description was created by Ellipsis for ba4dc61. It will automatically update as commits are pushed.

Copy link

vercel bot commented Feb 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 7, 2025 4:47am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 942ddc7 in 1 minute and 33 seconds

More details
  • Looked at 716 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. tools/build:190
  • Draft comment:
    The updated command block for /integ-tests/python correctly replaces 'poetry run' with 'uv run'. Verify that using 'BAML_LOG=info' in test mode is intended and consider safer alternatives to 'eval' for executing commands.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is mostly just stating what was changed and asking for verification, rather than pointing out actual issues. The eval concern seems speculative since eval is used throughout the file and appears to be an intentional pattern. The log level change from trace to info seems intentional given it's an explicit change in the diff.
    I could be wrong about the eval usage - it could be a legitimate security concern. The log level change could have unintended consequences I'm not considering.
    The eval usage is consistent throughout the file and would require a larger refactor to address. The log level change appears deliberate and asking for verification adds no value.
    This comment should be deleted as it mostly just asks for verification of intentional changes and raises speculative concerns without providing actionable feedback.
2. tools/build:191
  • Draft comment:
    The variable 'command' is initially set to 'true' but is immediately overwritten on the next line. Remove this redundant assignment to keep the script clean.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. tools/build:197
  • Draft comment:
    The test command here sets BAML_LOG to 'info', but previous (commented) lines suggested using 'trace'. Confirm whether 'info' is the desired logging level for test runs.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This comment violates our rules. It's asking for confirmation ("Confirm whether...") rather than pointing out a clear issue. The change in log level appears intentional - the author explicitly changed it. We should trust that the author knows what logging level they want. Additionally, commented out code is not a strong signal of what's correct - it's often left as reference or historical artifacts.
    Perhaps the change in log level could indicate a real issue that needs attention? Maybe trace logging was necessary for debugging important issues?
    If trace logging was truly required, the code wouldn't work and tests would fail. The author can easily change the log level back if needed. We shouldn't question every intentional change.
    Delete this comment. It violates our rule against asking for confirmations, and the log level change appears to be an intentional part of the changes.
4. integ-tests/python/pyproject.toml:5
  • Draft comment:
    The 'authors' field contains a placeholder value [{ name = "None" }]. It is recommended to update this with actual contributor information or remove it if not applicable.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This appears to be a test/integration test project based on the name and location. The author information is likely intentionally left as "None" since it's not a published package. The comment is suggesting a change that isn't strictly necessary and is more of a nice-to-have suggestion rather than a required fix.
    The author field could be important for maintenance and attribution purposes. Having accurate author information might be valuable even for internal test packages.
    While author information can be valuable, for an integration test package it's not critical. The comment doesn't point out a real issue that needs fixing.
    Delete the comment as it's not pointing out a clear issue that requires fixing, and is more of a suggestion about metadata that isn't critical for the package's functionality.
5. integ-tests/python/README.md:3
  • Draft comment:
    The README starts with an AI-generated note. Once the document is reviewed and verified, consider removing or updating this note so that end users are not confused by its provisional nature.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_8gxNfrtp0ne5Vuzb


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 5eb68e7 in 31 seconds

More details
  • Looked at 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. integ-tests/python/run_tests.sh:7
  • Draft comment:
    Ensure the 'uv' CLI is available in all target environments (CI/local) since the old 'poetry run' method is removed.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the author to ensure that a certain CLI tool is available in all environments, which is a form of asking for confirmation or verification. This violates the rule against asking the author to ensure something is tested or verified.
2. integ-tests/python/run_tests.sh:7
  • Draft comment:
    Switching from 'env -u CONDA_PREFIX poetry run' to 'uv run' removes the explicit unsetting of CONDA_PREFIX. Confirm that this change is intended and won’t introduce issues with lingering environment variables.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment is asking the PR author to confirm their intention regarding a change in command usage. It points out a specific change and its potential impact on environment variables, which is a valid concern. However, it does ask for confirmation, which is against the rules. The comment could be rephrased to suggest checking for issues without asking for confirmation.
3. integ-tests/python/run_tests.sh:8
  • Draft comment:
    Ensure that 'uv run' replicates the behavior previously provided by 'poetry run' for the baml-cli command without additional environment adjustments.
  • Reason this comment was not posted:
    Marked as duplicate.
4. integ-tests/python/run_tests.sh:11
  • Draft comment:
    Verify that replacing 'poetry run pytest' with 'uv run pytest' correctly preserves any environment and configuration settings needed for tests, especially given the removal of the 'env -u CONDA_PREFIX' in previous commands.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_myElvnLLcGV8BKke


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 377a13d in 1 minute and 4 seconds

More details
  • Looked at 288 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. tools/bump-version.py:1
  • Draft comment:
    Ensure the custom shebang '#!/usr/bin/env -S uv run --script' is compatible across all target systems.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. tools/bump-version.py:317
  • Draft comment:
    Avoid passing an empty string as an argument in bump2version calls; conditionally append '--allow-dirty' only if set.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. tools/bump-version.py:250
  • Draft comment:
    When using 'gh pr create', ensure commit_body with newlines is properly escaped to avoid shell parsing issues.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tools/bump-version.py:141
  • Draft comment:
    Consider clarifying the directory used for checking code generation changes; ensure 'engine/language_client_codegen/src' is up-to-date.
  • Reason this comment was not posted:
    Confidence changes required: 40% <= threshold 50%
    None
5. tools/bump-version.py:317
  • Draft comment:
    Avoid passing an empty string for the '--allow-dirty' flag. Construct the argument list conditionally to skip empty strings.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. tools/bump-version.py:1
  • Draft comment:
    Verify that the shebang '#!/usr/bin/env -S uv run --script' is supported on all target systems.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_FovikAhDcI9SrxWt


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

```

3. Build and install the Python client:
```bash
# Note: env -u CONDA_PREFIX is needed if using Conda
env -u CONDA_PREFIX poetry run maturin develop --manifest-path ../../engine/language_client_python/Cargo.toml
uv run maturin develop --uv --manifest-path ../../engine/language_client_python/Cargo.toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider clarifying instructions for Conda users; the previous 'env -u CONDA_PREFIX' was removed from the uv command.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e5e2e18 in 36 seconds

More details
  • Looked at 9 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. .mise.toml:5
  • Draft comment:
    Removal of the poetry version. Ensure docs/builds are updated.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. .mise.toml:1
  • Draft comment:
    Verify the [tools] section is correct after removal of poetry.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .mise.toml:5
  • Draft comment:
    Ensure removal of the poetry version line is intentional, as it could break any dependency on Poetry if still used.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_lQLJGAbOytTisi5o


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on aa6b7b6 in 30 seconds

More details
  • Looked at 10 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/primary.yml:126
  • Draft comment:
    Confirm that dependency installation is handled elsewhere. Removing 'poetry install' must be intentional.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm their intention regarding the removal of 'poetry install'. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
2. .github/workflows/primary.yml:126
  • Draft comment:
    Ensure removal of 'poetry install' is intentional. Confirm run_tests.sh (using uv) now handles dependency setup.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_u3ccFH5HTSJMtfDC


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on aad2bcb in 1 minute and 0 seconds

More details
  • Looked at 1035 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. .github/examples/bump_versions.yml:1
  • Draft comment:
    Removed the bump_versions workflow. Ensure that version bumping is now managed by the new uv-based processes.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. .github/examples/cli.yml:1
  • Draft comment:
    Removed the CLI workflow. Verify that CLI build and test steps are fully migrated to uv commands.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .github/examples/release_language_client_python.yaml:1
  • Draft comment:
    Removed the Python client release workflow file. Confirm that its functionality is now part of the uv integration.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. .github/examples/release_language_client_typescript.yaml:1
  • Draft comment:
    Removed the TypeScript client release workflow. Ensure that its responsibilities have been properly shifted to the new uv system.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. .github/examples/tests/chron-event.json:1
  • Draft comment:
    Removed test event JSON; verify that necessary integration test triggers are now handled via uv.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. .github/examples/tests/test-vscode-extention.sh:1
  • Draft comment:
    Removed VSCode extension test shell script. Confirm its logic has been replaced in the uv integration tests.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. .github/examples/tests/version-bump-event.json:1
  • Draft comment:
    Removed the version bump event JSON file. Ensure that the version bump testing is now covered in the uv flow.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. .github/examples/vscode_ext.yml:1
  • Draft comment:
    Removed the VSCode extension workflow. Make sure its build and release steps are fully migrated to the new uv-based integration pipeline.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. .github/examples/bump_versions.yml:1
  • Draft comment:
    Removed bump_versions.yml workflow. Ensure that any documentation or CI references to version bumping have been updated to match the new integration approach.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. .github/examples/cli.yml:1
  • Draft comment:
    Legacy CLI workflow configuration has been removed. Confirm that all documentation and CI pipelines are updated to use the new uv-based commands.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. .github/examples/release_language_client_python.yaml:1
  • Draft comment:
    Removed the Python client release workflow. Ensure that the new uv-based integration tests and release commands now cover these responsibilities.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. .github/examples/release_language_client_typescript.yaml:1
  • Draft comment:
    The release workflow for the TypeScript language client has been removed. Verify that all necessary release steps are now incorporated elsewhere.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. .github/examples/tests/chron-event.json:1
  • Draft comment:
    Example chron-event JSON has been removed. Make sure any tests or documentation referencing this payload are updated.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. .github/examples/tests/test-vscode-extention.sh:1
  • Draft comment:
    Removed the VSCode extension test shell script. Confirm that the new uv-based commands fully replace this functionality.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. .github/examples/tests/version-bump-event.json:1
  • Draft comment:
    Removed sample version bump event JSON. Ensure related CI or documentation is updated accordingly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. .github/examples/vscode_ext.yml:1
  • Draft comment:
    VSCode extension workflow has been entirely removed. Confirm that the new uv-based integration tests address VSCode extension builds and that documentation reflects this change.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_1nabX22xYvpR0KAT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 69396b6 in 57 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/primary.yml:118
  • Draft comment:
    Consider pinning 'astral-sh/setup-uv@v5' to a specific commit hash to enhance build reproducibility and security.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While pinning to commit hashes can improve security, it's a tradeoff with maintainability. The current file consistently uses version tags for all actions. The v5 tag follows the same pattern as other actions in the file. Pinning to commit hashes would make updates more difficult and goes against the established pattern in this codebase.
    The comment raises a valid security consideration. Malicious actors could potentially compromise a tagged version.
    The risk is mitigated by GitHub's security features for actions, and the consistent use of version tags throughout the file suggests this is an intentional maintainability choice.
    Delete the comment as it goes against the established patterns in the codebase and the tradeoff between security and maintainability has already been decided in favor of using version tags.
2. .github/workflows/primary.yml:118
  • Draft comment:
    Added the uv setup step. Verify if the v5 tag suffices or if you need to pin to a specific commit hash for security.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_OhP98QBtMJ8qVkAY


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on c746f09 in 42 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. integ-tests/python/run_tests.sh:7
  • Draft comment:
    Add a comment explaining the purpose of 'uv sync' for clarity, especially its role in setting up the environment.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. integ-tests/python/run_tests.sh:7
  • Draft comment:
    Consider adding a brief comment explaining the purpose of 'uv sync' for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_Ly45GvbVJKTuEmcg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on ba4dc61 in 1 minute and 3 seconds

More details
  • Looked at 58 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. integ-tests/python/pyproject.toml:23
  • Draft comment:
    Enabling packaging with 'package = true' under [tool.uv] is intentional? Confirm that this change aligns with the intended workflow.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. integ-tests/python/uv.lock:386
  • Draft comment:
    The uv.lock file appears to be fully regenerated with detailed pins. Ensure it’s kept up-to-date and document the regeneration process if it is auto-generated.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. integ-tests/python/pyproject.toml:20
  • Draft comment:
    Confirm that setting [tool.uv].package = true is intentional for the new uv‐based packaging setup.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. integ-tests/python/uv.lock:386
  • Draft comment:
    The 'python-integ-tests' package now uses editable mode (source = { editable = "." }). Verify that this change aligns with the intended development workflow.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. integ-tests/python/uv.lock:1
  • Draft comment:
    The auto-generated uv.lock now lists detailed platform-specific wheels. Please ensure that no unintended dependency upgrades or hash changes slipped in during the switch from Poetry.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_AnNWgHEILBNHtTDL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@sxlijin sxlijin added this pull request to the merge queue Feb 7, 2025
Merged via the queue into canary with commit 62fba91 Feb 7, 2025
11 checks passed
@sxlijin sxlijin deleted the sam/uv-integ-tests branch February 7, 2025 04:52
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.

1 participant