-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix(CI_Pipeline): resolve dependency issues causing build failures #1508
Conversation
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 to me! Reviewed everything up to 3e6df5f in 11 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. extensions/ee/connectors/bigquery/pyproject.toml:10
- Draft comment:
Switching to an alpha versionpandasai = "^3.0.0a0"
might introduce instability. Ensure this is intentional and tested. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_MEuQJa26wBTQOQxy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 to me! Incremental review on b470344 in 21 seconds
More details
- Looked at
61
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. pandasai/__init__.py:19
- Draft comment:
The import order was corrected to follow PEP 8 guidelines, which is a good practice. - Reason this comment was not posted:
Confidence changes required:0%
The import order was changed to follow PEP 8 guidelines, which is a good practice.
2. pandasai/config.py:6
- Draft comment:
The import order was corrected to follow PEP 8 guidelines, which is a good practice. - Reason this comment was not posted:
Confidence changes required:0%
The import order was changed to follow PEP 8 guidelines, which is a good practice.
3. pandasai/helpers/request.py:10
- Draft comment:
The import statement forload_dotenv
was moved to follow PEP 8 guidelines, which is a good practice. - Reason this comment was not posted:
Confidence changes required:0%
The import statement forload_dotenv
was moved to follow PEP 8 guidelines, which is a good practice.
Workflow ID: wflow_dw25Rdm3WCdvHhzT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 to me! Incremental review on 972ccd1 in 9 seconds
More details
- Looked at
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_uqvgBX5f8v8snDcm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Skipped PR review on 170c0f9 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
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 to me! Incremental review on 6d5ddfa in 11 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. .github/workflows/ci.yml:19
- Draft comment:
Using 'sudo' might not be necessary in all environments and could fail if the runner lacks sudo privileges. Ensure the runner has the necessary permissions or handle potential failures gracefully. - Reason this comment was not posted:
Confidence changes required:50%
The cleanup step is being executed for non-Windows environments, which is fine. However, the use of 'sudo' might not be necessary in all environments, and it could fail if the runner does not have sudo privileges. It's better to ensure that the runner has the necessary permissions or handle the failure gracefully.
2. .github/workflows/ci.yml:17
- Draft comment:
The phrase 'Clean up instance space' is grammatically correct, but could be more descriptive. Consider 'Clean up disk space on instance' for clarity. - Reason this comment was not posted:
Confidence changes required:33%
The phrase 'Clean up instance space' is grammatically correct, but it could be more descriptive. However, it is not a significant issue.
Workflow ID: wflow_1PRc9lwSMU6HpnxD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 to me! Incremental review on 71c94a5 in 28 seconds
More details
- Looked at
347
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. .github/workflows/ci-core.yml:51
- Draft comment:
The installation of the 'future' package seems unnecessary as it is not used later in the script. Consider removing this step to streamline the workflow. - Reason this comment was not posted:
Comment did not seem useful.
2. .github/workflows/ci-extensions.yml:68
- Draft comment:
Consider rephrasing for clarity and consistency:
Write-Host "Executing tests for $($_.FullName)"
- Reason this comment was not posted:
Confidence changes required:50%
The error message in the script is clear and concise, but the use of 'Running tests for' could be improved for better clarity and consistency.
Workflow ID: wflow_bLYz6mrrx8B3RULE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v3 #1508 +/- ##
==============================================
- Coverage 79.09% 72.62% -6.47%
==============================================
Files 149 64 -85
Lines 6013 2214 -3799
==============================================
- Hits 4756 1608 -3148
+ Misses 1257 606 -651
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
❌ Changes requested. Incremental review on 4669f39 in 1 minute and 0 seconds
More details
- Looked at
113
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:57
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Installing dependencies for $dir..."
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The suggestion is purely stylistic and doesn't affect functionality. Looking at lines 62, 75, 80, 93, 98, 113, 125, and 137, none of the other similar log messages use ellipses. Adding one here would actually make this message inconsistent with the others. The current format is clear and concise.
Perhaps adding ellipses could make it clearer that this is an ongoing process that will take some time?
The ongoing nature of the process is already clear from the context and structure of the script. Adding ellipses would create inconsistency with other similar messages in the file.
This comment should be deleted as it suggests a purely stylistic change that would actually reduce consistency with other similar messages in the file.
2. .github/workflows/ci-extensions.yml:62
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Running tests for $dir..."
- Reason this comment was not posted:
Marked as duplicate.
3. .github/workflows/ci-extensions.yml:75
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Installing dependencies for $dir..."
- Reason this comment was not posted:
Marked as duplicate.
4. .github/workflows/ci-extensions.yml:80
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Running tests for $dir..."
- Reason this comment was not posted:
Marked as duplicate.
5. .github/workflows/ci-extensions.yml:93
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Installing dependencies for $dir..."
- Reason this comment was not posted:
Marked as duplicate.
6. .github/workflows/ci-extensions.yml:98
- Draft comment:
Consider rephrasing for consistency and clarity:
echo "Running tests for $dir..."
- Reason this comment was not posted:
Marked as duplicate.
7. .github/workflows/ci-extensions.yml:113
- Draft comment:
Consider rephrasing for consistency and clarity:
Write-Host "Running tests for $($_.FullName)..."
- Reason this comment was not posted:
Marked as duplicate.
8. .github/workflows/ci-extensions.yml:125
- Draft comment:
Consider rephrasing for consistency and clarity:
Write-Host "Running tests for $($_.FullName)..."
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_in34A1ztgEsjVcFt
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.
@@ -137,11 +140,15 @@ jobs: | |||
Pop-Location |
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 test execution step is missing for enterprise extensions on Windows. Add poetry run pytest tests/
after installing dependencies.
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.
❌ Changes requested. Incremental review on 098d685 in 51 seconds
More details
- Looked at
50
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:162
- Draft comment:
Switching from Poetry to pip for installingcoverage
can lead to inconsistencies. Ensure this change is intentional and won't cause issues with dependency management. - Reason this comment was not posted:
Comment did not seem useful.
2. .github/workflows/ci-extensions.yml:67
- Draft comment:
The phrase 'Removing envs' should be 'Removing environments' for clarity and correctness. This applies to lines 90 and 113 as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a very minor suggestion about log message wording. "envs" is a commonly used shorthand in the context of Python virtual environments, especially with tools like Poetry. The message is clear enough for developers who will be reading CI logs. This seems like an unnecessarily pedantic change that doesn't improve code quality or clarity.
The comment does point out a consistency issue in terminology, and "environments" is more formal and clearer for newcomers.
While more formal, changing CI log messages from a common technical shorthand to a longer form doesn't provide meaningful value and goes against the rule about not making purely informative comments or obvious suggestions.
Delete this comment as it suggests a trivial change to log messages that doesn't improve code quality or fix any real issues.
Workflow ID: wflow_50dVBZ5EYAbCgfQz
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.
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 to me! Incremental review on 234c053 in 33 seconds
More details
- Looked at
31
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:70
- Draft comment:
Ensure that the--all
flag is supported in the version of Poetry being used, as this change relies on it. - Reason this comment was not posted:
Confidence changes required:50%
The change from usingpoetry env list --all | awk '{print $1}' | xargs -n 1 poetry env remove
topoetry env remove --all
is a simplification and should work as intended. However, it's important to ensure that the--all
flag is supported in the version of Poetry being used.
2. .github/workflows/ci-extensions.yml:67
- Draft comment:
Consider changing 'Removing envs' to 'Removing environments' for clarity and grammatical correctness. This applies to lines 67, 90, and 113. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ZhSvHrjcBI85r4xZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
This PR restructures CI workflows, updates dependencies, and fixes import order issues to resolve build failures.
ci-core.yml
for core tests on multiple OS and Python versions.ci.yml
toci-extensions.yml
and update job names.pandasai
dependency to^3.0.0a0
inbigquery/pyproject.toml
.__init__.py
,config.py
, andrequest.py
.test_convert_df_to_yml()
intest_dataframe_serializer.py
.This description was created by for 234c053. It will automatically update as commits are pushed.