-
Notifications
You must be signed in to change notification settings - Fork 331
Add a pool timeout to try to fix open File descriptor issue like deno #2205
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
🌿 Preview your docs: https://boundary-preview-141278b1-3237-4a1b-9210-af36ea982ff4.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed everything up to 5ad7fa5 in 1 minute and 57 seconds. Click for details.
- Reviewed
35lines 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. engine/baml-runtime/Cargo.toml:140
- Draft comment:
Updated reqwest version; please verify that the new 'native-tls-alpn' feature is compatible with all usages. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/baml-runtime/src/request/mod.rs:24
- Draft comment:
Mixing Duration types may be confusing. Use a consistent Duration (e.g. std::time::Duration) for all reqwest timeout settings. - Reason this comment was not posted:
Comment was on unchanged code.
3. engine/baml-runtime/src/request/mod.rs:45
- Draft comment:
The variable 'cb' declared inside the cfg_if block isn’t in scope outside it. Bind 'cb' in a unified way (e.g. using an if/else expression) so it's available for building the client. - 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. engine/baml-runtime/src/request/mod.rs:17
- Draft comment:
Typo: Consider capitalizing 'python' to 'Python' in the comment as it is a proper noun. - 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 is a purely stylistic comment about documentation. It doesn't affect code functionality or quality. While technically correct that Python should be capitalized, this kind of nitpick about comment formatting doesn't meet our bar for "clearly a code change required". Our rules explicitly state not to make purely informative comments or unimportant suggestions. Perhaps maintaining consistent and professional documentation style is more important than I'm giving it credit for? Some teams have strict documentation standards. While documentation standards matter, our rules clearly state to only keep comments that require clear code changes. This minor capitalization issue doesn't meet that threshold. Delete this comment. It's a minor documentation style suggestion that doesn't meet our criteria for required code changes.
Workflow ID: wflow_gkDhXONpVSNpEHfW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
🌿 Preview your docs: https://boundary-preview-18761c71-c19f-4507-9079-b6b192d311f9.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed 3ccc8c0 in 1 minute and 48 seconds. Click for details.
- Reviewed
292lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft 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. engine/baml-runtime/Cargo.toml:29
- Draft comment:
Workspace dependencies: Verify that all dependencies marked with.workspace = true(e.g. anyhow, baml-ids, etc.) are consistently defined in the workspace. Also, ensure no duplicate version specifications occur (e.g. 'either' appears both as workspace (line 45) and explicitly in dev-dependencies (line 207)). - 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. engine/baml-runtime/Cargo.toml:75
- Draft comment:
Tokio features vary by target: In the main dependency, 'tokio' is set with minimal features (macros, time) while in non‑wasm target it uses 'full'. Confirm if this split is intentional. - 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. engine/baml-runtime/Cargo.toml:53
- Draft comment:
Commented dependency: Remove or clarify the commented-out 'instant' and 'tower' lines if they’re no longer required. - 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. engine/baml-runtime/Cargo.toml:126
- Draft comment:
AWS SDK comments: The notes regarding AWS dependencies (lines 126–131 and 169–174) are useful; ensure they remain updated as SDK issues evolve. - 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. engine/baml-runtime/Cargo.toml:194
- Draft comment:
Features section: The defaults are set to 'skip-integ-tests'. Consider adding a brief comment explaining the rationale for these feature settings. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking for an explanation of the rationale behind a specific feature setting. This falls under asking the PR author to explain or confirm their intention, which is against the rules. The comment does not provide a specific code suggestion or identify a potential issue with the code itself.
6. engine/baml-runtime/Cargo.toml:200
- Draft comment:
Dev-dependencies consistency: Review dependencies declared both in target-specific sections and in [dev-dependencies] (e.g., 'criterion' and 'either') to avoid version conflicts. - 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_zfROslddc2kWJNwl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
🌿 Preview your docs: https://boundary-preview-2c565f4d-8018-4ac5-b4d9-9ab5e755f66b.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed 9bcb5cf in 1 minute and 46 seconds. Click for details.
- Reviewed
283lines 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. engine/baml-runtime/Cargo.toml:1
- Draft comment:
Overall, this Cargo.toml is well‐organized and makes good use of workspace dependency declarations and target-specific sections (for wasm32 vs non-wasm32). The comments help clarify nonstandard choices (e.g. AWS SDK and ring vs aws_lc_rs issues). - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. engine/baml-runtime/Cargo.toml:29
- Draft comment:
Several dependencies are declared with the.workspace = truesyntax; ensure that your workspace root defines these unified versions to avoid conflicts. This approach is good for monorepo management. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. engine/baml-runtime/Cargo.toml:75
- Draft comment:
There is a differing set of features for dependencies liketokio(e.g. in the main dependencies vs target-specific overrides). It may be helpful to add a comment justifying why the wasm and non-wasm targets require different feature sets. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. engine/baml-runtime/Cargo.toml:196
- Draft comment:
The [features] section defines a feature nameddefaults, but by Cargo convention the default feature is specified under thedefaultkey. Consider renaming it if the intent is to define the default set of features. - 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_C8KWkj5Kvo6htAwV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Theres no pool technically but i saw this on Deno as well so we can try this out.
Important
Set pool parameters in
builder()inmod.rsto prevent stalling, inspired by similar issues in Deno and other projects.builder()inmod.rs, setpool_max_idle_per_host(0)andpool_idle_timeout(Duration::from_nanos(1))to prevent stalling in Python.reqwest,deno,hyper, andazure-sdk-for-rustfor context.builder()to explain the rationale for pool settings and link to related issues.This description was created by
for 9bcb5cf. You can customize this summary. It will automatically update as commits are pushed.