update deps versions #5703
Merged
update deps versions #5703
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 6bc8252 in 1 minute and 25 seconds. Click for details.
- Reviewed
2231lines of code in6files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/Cargo.toml:11
- Draft comment:
The features list is extensive and the dependencies are well‐organized. Consider adding inline comments to group related features (e.g. enterprise, embedding and triggers) for better readability. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. backend/windmill-api/src/embeddings.rs:157
- Draft comment:
The asynchronous model loading and embedding creation logic is clear. Consider adding additional inline comments to explain the fallback logic in load_model_files – in particular, clarify the bucket vs hub retrieval flow. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. backend/windmill-common/src/ee.rs:1
- Draft comment:
The enterprise-related error and license key handling functions are stubbed out. Ensure that when integrating the closed‐source implementations the error handling and logging are comprehensive. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. backend/windmill-api/Cargo.toml:18
- Draft comment:
Ensure that the dependency keys in the 'embedding' feature (e.g. tinyvector, hf-hub, tokenizers, candle-core, etc.) are consistent with the updated workspace versions. Double‐check that any version bumps in workspace dependencies are reflected here. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to double-check and ensure consistency in dependency versions, which violates the rule against asking for confirmation or double-checking. It does not provide a specific suggestion or point out a specific issue.
5. backend/windmill-api/src/embeddings.rs:161
- Draft comment:
The model repository string 'thenlper/gte-small' is hard-coded. Consider making it configurable or document its purpose for future maintenance. - 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. backend/windmill-api/src/embeddings.rs:237
- Draft comment:
The unsafe block calling VarBuilder::from_mmaped_safetensors requires explicit justification. Please document why it is safe and what invariants are assumed. - 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. backend/windmill-api/src/embeddings.rs:244
- Draft comment:
In the create_embedding function, tokenization errors are mapped via Error::msg. Consider whether more precise error handling or logging is beneficial. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
8. backend/windmill-api/src/embeddings.rs:590
- Draft comment:
The normalize_l2 function performs L2 normalization on a tensor. Consider adding inline comments about tensor dimensions and verify that the broadcasting/division works as intended. - 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. backend/windmill-api/src/embeddings.rs:612
- Draft comment:
In update_embeddings_db, errors when creating the new embeddings DB are only logged. Consider whether failing to update should trigger further actions or propagate the error. - 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. backend/windmill-common/src/ee.rs:1
- Draft comment:
Ensure that the enterprise feature flag is enabled when these functions are meant to be used. The file relies on #[cfg(feature = "enterprise")], so proper testing in enterprise builds is essential. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the enterprise feature flag is enabled and that proper testing is done in enterprise builds. This violates the rule against asking the author to ensure testing or confirm intentions. The comment does not provide a specific code suggestion or ask for a specific test to be written, so it should be removed.
11. backend/windmill-common/src/ee.rs:51
- Draft comment:
Enterprise functions (e.g. send_critical_alert, maybe_renew_license_key_on_start, etc.) are stubs with 'not open source' comments. Ensure that proper implementations and error handling are provided in production enterprise builds. - 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_bPSZjKo4iFHlBtt8
You can customize 
Deploying windmill with
|
| Latest commit: |
783db3a
|
| Status: | ⚡️ Build in progress... |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 50cfdda in 2 minutes and 0 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/src/monitor.rs:32
- Draft comment:
The new enterprise-specific import for low_disk_alerts is conditionally compiled and consistent with the other enterprise imports. Consider grouping all enterprise imports in one block for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. backend/src/monitor.rs:592
- Draft comment:
Typo found in comment: 'parse datetime frome file xxxx.yyyy-MM-dd-HH-mm'. Please change 'frome' to 'from'. - 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. backend/src/monitor.rs:178
- Draft comment:
Typo in error message: 'default tag per workpsace' should be 'default tag per workspace'. - 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. backend/src/monitor.rs:930
- Draft comment:
Typographical error in log message: 'Succesfully deleted {} from disk' should be 'Successfully deleted {} from disk'. - 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_TosYi1a7Hp4yoLiW
You can customize
| @@ -30,7 +30,7 @@ use windmill_api::{ | |||
|
|
|||
| #[cfg(feature = "enterprise")] | |||
There was a problem hiding this comment.
Consider merging the two #[cfg(feature = "enterprise")] imports into a single statement for clarity.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5f1a1d4 in 2 minutes and 35 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/src/monitor.rs:1
- Draft comment:
Since this PR updates dependency versions, please double-check that all API usages (for instance in functions related to SMTP, S3, and OAuth) remain compatible with the new versions. - 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. backend/src/monitor.rs:30
- Draft comment:
The removed import for low_disk_alerts might cause a compile error since the function is still used later (e.g., in the low_disk_alerts_f async block). Ensure its usage is removed or update the import to match the new dependency version. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The key insight is that the function is used in non-enterprise code (monitor_db), so it MUST be available without the enterprise feature flag. The PR author is actually fixing a bug by removing the enterprise feature flag - the function needs to be available in non-enterprise builds. The comment is incorrect in suggesting this will cause compilation errors. Could there be other usages of low_disk_alerts that I'm missing? Could the function definition itself be feature-gated elsewhere? Even if the function is feature-gated elsewhere, that would mean the current code is broken since it tries to use the function in non-enterprise code. The PR is fixing this inconsistency. The comment should be deleted. The PR is correctly removing the enterprise feature flag since the function is used in non-enterprise code.
3. backend/src/monitor.rs:591
- Draft comment:
Typo: Please change 'parse datetime frome file xxxx.yyyy-MM-dd-HH-mm' to 'parse datetime from file xxxx.yyyy-MM-dd-HH-mm'. - 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. backend/src/monitor.rs:177
- Draft comment:
Typo: Change 'default tag per workpsace' to 'default tag per workspace' in the error message. - 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. backend/src/monitor.rs:181
- Draft comment:
Typo: Change 'default tag per workpsace workspaces' to 'default tag per workspace workspaces' in the error message. - 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. backend/src/monitor.rs:933
- Draft comment:
Typo: Change 'Succesfully deleted {} from disk' to 'Successfully deleted {} from disk'. - 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. backend/src/monitor.rs:947
- Draft comment:
Typo: Change 'Succesfully deleted {} from object store' to 'Successfully deleted {} from object store'. - 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_WMxEDmwNZCqlE8B8
You can customize
| @@ -30,7 +30,6 @@ use windmill_api::{ | |||
|
|
|||
| #[cfg(feature = "enterprise")] | |||
| use windmill_common::ee::{jobs_waiting_alerts, worker_groups_alerts}; | |||
|
|
|||
| use windmill_common::ee::low_disk_alerts; | |||
There was a problem hiding this comment.
The imports for enterprise features use a #[cfg(feature = "enterprise")] on jobs_waiting_alerts and worker_groups_alerts, but low_disk_alerts is imported without the same cfg guard. For consistency (and to avoid accidental inclusion in non‐enterprise builds), consider wrapping low_disk_alerts in #[cfg(feature = "enterprise")].
Suggested change
| use windmill_common::ee::low_disk_alerts; | |
| #[cfg(feature = "enterprise")] use windmill_common::ee::low_disk_alerts; |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c078c59 in 2 minutes and 42 seconds. Click for details.
- Reviewed
57lines of code in2files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/src/monitor.rs:1356
- Draft comment:
Renaming the parameter to_worker_modeis a clear way to silence unused warnings. Confirm that the renamed variable is consistently used. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. backend/src/monitor.rs:1419
- Draft comment:
The use of #[cfg(not(feature = "enterprise"))] block with an empty tuple forlow_disk_alertsinvocation is clear; ensure that this behavior is what you want in non-enterprise builds. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. backend/windmill-common/src/ee.rs:103
- Draft comment:
The stub implementation of 'low_disk_alerts' is acceptable as a placeholder for non open-source components; ensure that its interface remains in sync with monitor.rs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. backend/src/monitor.rs:1418
- Draft comment:
The async block for low_disk_alerts (enterprise-only) is correctly guarded with cfg attributes. Consider renaming _worker_mode to worker_mode for clarity when it’s actually used. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The underscore prefix in Rust indicates that a variable is intentionally unused. The diff shows the parameter was renamed fromworker_modeto_worker_mode, which is actually the correct Rust idiom for marking unused parameters. The comment suggests going backwards from this good practice. The parameter is used in the enterprise feature path but unused in the non-enterprise path, so marking it as potentially unused with _ is appropriate. Could the parameter be used elsewhere in the codebase that I haven't seen? Is there a reason it needs to be kept as a non-underscore name? No - the underscore prefix is the correct Rust idiom here. Even if the parameter is used in some code paths (enterprise), marking it with _ is still appropriate since it's unused in other paths (non-enterprise). The compiler will not complain about using a _prefixed variable. The comment should be deleted. The underscore prefix is the correct Rust idiom for marking parameters that may be unused in some code paths. Removing the underscore would go against Rust best practices.
5. backend/windmill-common/src/ee.rs:102
- Draft comment:
low_disk_alerts is implemented as a stub. Ensure the enterprise build provides the full implementation and add documentation to clarify this. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. backend/windmill-common/src/ee.rs:22
- Draft comment:
get_license_plan always returns Community. If this is intentional for the open source version, please document that behavior as a default placeholder. - 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. backend/src/monitor.rs:592
- Draft comment:
Typo: Replace 'frome' with 'from' in the comment (currently: 'parse datetime frome file xxxx.yyyy-MM-dd-HH-mm'). - 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. backend/src/monitor.rs:178
- Draft comment:
Typo: In the error message string 'Error loading default tag per workpsace: {e:#}', change 'workpsace' to 'workspace'. - 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. backend/src/monitor.rs:182
- Draft comment:
Typo: In the error message string 'Error loading default tag per workpsace workspaces: {e:#}', fix 'workpsace' to 'workspace'. - 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. backend/src/monitor.rs:931
- Draft comment:
Typo: Change 'Succesfully deleted {} from disk' to 'Successfully deleted {} from disk'. - 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. backend/src/monitor.rs:946
- Draft comment:
Typo: Correct 'Succesfully deleted {} from object store' to 'Successfully deleted {} from object store'. - 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. backend/src/monitor.rs:159
- Draft comment:
Typo/grammar: Update the error message 'Could loading critical error emails setting: {:?}' to something like 'Error loading critical error emails setting: {:?}'. - 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_NXVIOnIwKvqbZ8ZN
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5dce832 in 50 seconds. Click for details.
- Reviewed
9lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/ee-repo-ref.txt:1
- Draft comment:
Verify the new dependency commit hash; ensure it matches the intended version. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. backend/ee-repo-ref.txt:1
- Draft comment:
Verify the updated commit hash is correct and corresponds to a stable dependency release. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_jzW8dnu4IDSe5xnu
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8886dfb in 1 minute and 28 seconds. Click for details.
- Reviewed
23lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/ee-repo-ref.txt:1
- Draft comment:
Consider adding a trailing newline to the file. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. backend/windmill-common/src/ee.rs:98
- Draft comment:
Removal of 'low_disk_alerts' function: ensure its removal is intentional and update call sites if necessary. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. backend/ee-repo-ref.txt:1
- Draft comment:
Dependency commit updated to 868ccad87afb804fe22818ecd3d5a091199bcdbf. Verify compatibility with consumers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. backend/windmill-common/src/ee.rs:101
- Draft comment:
Removed low_disk_alerts implementation. Confirm that its removal won't break any enterprise 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.
Workflow ID: wflow_7JgQhWm1LyE4Xecd
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 783db3a in 1 minute and 48 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-common/src/ee.rs:103
- Draft comment:
Consider using a placeholder likeunimplemented!()if this function should not be called in the open source version, instead of an empty body. This clarifies intent. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/windmill-common/src/ee.rs:103
- Draft comment:
Consider adding a doc comment for low_disk_alerts to explain its purpose and parameters. Also, if the '_workers' parameter is read-only, consider using a slice (&[String]) to avoid unnecessary allocations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_J8eG16JxECw1hqzh
You can customize
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Update dependencies and adjust code for compatibility and security improvements.
arrow,datafusion,candle-core,candle-transformers,candle-nn,chrono,half,lexical-core,object_store,parquet,sqlparser, andzipversions inCargo.lock.halffromCargo.tomland updatedatafusionandobject_storeversions.forward()call inModelInstanceinembeddings.rsto includeNoneas the third argument.halffeature fromwindmill-api/Cargo.toml.monitor_db()function inmonitor.rsto use_worker_modeinstead ofworker_mode.ee-repo-ref.txtto a new commit hash.worker.rsto improve error handling and feature gating.This description was created by
for 783db3a. You can customize this summary. It will automatically update as commits are pushed.