Add support for manually configuring log level via code #1600
+2,518
−631
Conversation
This file contains 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
resetEnvVars now also sets log levels! Fixes #805
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to a5fb87d in 3 minutes and 27 seconds
More details
- Looked at
2554lines of code in42files - Skipped
0files when reviewing. - Skipped posting
47drafted comments based on config settings.
1. engine/baml-runtime/src/lib.rs:140
- Draft comment:
Consider using a more explicit error message when parsing environment variables in set_from_env. Also, include logging for successful updates if 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%
The comment is suggesting improvements to error handling in an external function that we can't see. Without seeing the implementation ofbaml_log::set_from_env, we can't verify if the error messages are actually insufficient. The comment is speculative and assumes the current error handling is inadequate without evidence. Additionally, logging suggestions are more of an implementation detail that could vary based on the logging strategy of the codebase.
I could be wrong if there are known issues with the error messages from this function that the reviewer is aware of. The logging suggestion could also be part of established logging patterns in the codebase.
Even if there are known issues, the comment is not specific enough about what's wrong with the current error messages or what specific improvements are needed. It reads more like a general suggestion rather than pointing out a concrete problem.
The comment should be deleted as it makes assumptions about an external function's implementation and provides vague suggestions without clear evidence of problems.
2. engine/baml-runtime/src/lib.rs:160
- Draft comment:
Document why cloning env_vars is necessary before passing to set_from_env. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for documentation on why a specific action (cloningenv_vars) is necessary. This is a request for clarification or explanation, which violates the rule against asking the PR author to explain or confirm their intention. Therefore, this comment should not be approved.
3. engine/baml-runtime/src/tracing/api_wrapper/mod.rs:299
- Draft comment:
Ensure failure to serialize log events is handled gracefully instead of using unwrap_or_default. - Reason this comment was not posted:
Comment was on unchanged code.
4. engine/baml-runtime/src/tracing/mod.rs:428
- Draft comment:
Improve debug information for span ID mismatch errors when finishing spans. - 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/src/tracingv2/publisher/publisher.rs:100
- Draft comment:
Remove or document the commented-out HTTP upload block to clarify future work. - 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. engine/baml-runtime/src/tracingv2/storage/storage.rs:90
- Draft comment:
Consider replacing the panic on ref_count underflow with a proper error log for production robustness. - 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. engine/cli/Cargo.toml:16
- Draft comment:
Verify that the strict lints (dead_code, unused_imports, unused_variables) align with the overall project style. - 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. engine/cli/src/main.rs:5
- Draft comment:
Ensure errors from baml_log::init are properly reported during CLI startup. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is asking the author to ensure that errors from a specific function are properly reported. This is similar to asking the author to ensure something is tested or verified, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it more of a general reminder.
9. engine/language_client_codegen/Cargo.toml:10
- Draft comment:
Ensure dependency versions for baml-log and internal-baml-codegen remain in sync to avoid compatibility 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.
10. engine/language_client_codegen/src/lib.rs:62
- Draft comment:
Consider adding unit tests for relative_path_to_baml_src to cover edge cases. - 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. engine/language_client_codegen/src/lib.rs:114
- Draft comment:
Provide more specific context when serialization fails in file_map. - 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. engine/language_client_codegen/src/python/templates/globals.py.j2:37
- Draft comment:
Log a warning if reset_baml_env_vars fails instead of silently swallowing 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.
13. engine/language_client_python/Cargo.toml:35
- Draft comment:
Verify that 'abi3-py38' meets your minimum Python version requirements. - 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. engine/language_client_python/python_src/baml_py/baml_py.pyi:9
- Draft comment:
Ensure the type signature for set_log_level aligns with runtime expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that the type signature aligns with runtime expectations, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to ensure or verify things. It does not provide a specific suggestion or point out a specific issue.
15. engine/language_client_python/src/errors.rs:75
- Draft comment:
Consider using more descriptive error messages in BamlError::from_anyhow, and cover all error variants. - Reason this comment was not posted:
Comment was on unchanged code.
16. engine/language_client_python/src/lib.rs:77
- Draft comment:
The custom SIGINT handling using ctrlc is a hack; ensure potential side effects with Python's signal system are documented. - Reason this comment was not posted:
Comment was on unchanged code.
17. engine/language_client_python/src/parse_py_type.rs:308
- Draft comment:
When encountering an unsupported type, consider returning a structured error instead of a debug string. - 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.
18. engine/language_client_python/src/types/audio.rs:56
- Draft comment:
Add test cases for pickle serialization/deserialization as indicated in getnewargs. - 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.
19. engine/language_client_python/src/types/image.rs:39
- Draft comment:
Additional logging on failures when the image type does not match is recommended. - 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.
20. engine/language_client_python/src/types/log_collector.rs:107
- Draft comment:
Ensure potential locking issues are monitored when wrapping inner function logs in new FunctionLog objects. - 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.
21. engine/language_client_python/src/types/log_collector.rs:228
- Draft comment:
Verify that converting calls to JS array properly handles error cases; test both LLMCall and LLMStreamCall in TypeScript. - 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.
22. engine/language_client_ruby/ext/ruby_ffi/Cargo.toml:23
- Draft comment:
Ensure using rb-sys version '0.9.99' is intentional and compatible with future releases. - 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.
23. engine/language_client_ruby/ext/ruby_ffi/src/lib.rs:26
- Draft comment:
Verify that the blocking current-thread runtime is correctly configured to avoid Ruby panics. - 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.
24. engine/language_client_ruby/ext/ruby_ffi/src/lib.rs:110
- Draft comment:
Provide additional context in error messages when converting Ruby Hash to JSON in call_function. - 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.
25. engine/language_client_typescript/Cargo.toml:27
- Draft comment:
Verify that napi features 'tokio_rt' match the Tokio runtime configuration. - 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.
26. engine/language_client_typescript/src/lib.rs:12
- Draft comment:
Ensure that initialization failures in run_cli are logged in detail. - 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.
27. engine/language_client_typescript/src/types/log_collector.rs:69
- Draft comment:
Verify that converting inner recursive data structures into JS arrays correctly handles lifetimes. - 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.
28. engine/language_client_typescript/src/types/span.rs:25
- Draft comment:
In BamlSpan::new, add logging or error handling if args are not a map to aid debugging. - 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.
29. integ-tests/python/baml_client/globals.py:43
- Draft comment:
Consider aliasing long global variable names to shorter ones in comments to improve readability. - Reason this comment was not posted:
Comment was on unchanged code.
30. integ-tests/python/tests/memory_test.py:67
- Draft comment:
Consider adding assertions for specific memory usage trends during long-running 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.
31. integ-tests/python/tests/test_collector.py:15
- Draft comment:
Document potential race conditions when using Collector.__function_span_count() concurrently. - 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.
32. integ-tests/python/baml_client/globals.py.j2:37
- Draft comment:
The monkeypatch for dotenv.load_dotenv looks fine; however, ensure that any exceptions raised in reset_baml_env_vars are appropriately handled (you’re currently swallowing BamlError). Consider logging unexpected errors for easier debugging. - 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.
33. integ-tests/python/tests/memory_test.py:55
- Draft comment:
Memory usage test function prints and logs memory statistics. Ensure that the sampling interval and sleep durations are appropriate for varied environments to avoid flaky 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.
34. integ-tests/python/tests/test_collector.py:127
- Draft comment:
Several tests use gc.collect() and assert Collector.__function_span_count() > 0. The TODO notes about potential retention when no FunctionLog PYO3 objects are created should be addressed or tracked to avoid flaky behavior in a parallel test scenario. - 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.
35. engine/language_client_python/src/parse_py_type.rs:305
- Draft comment:
In the unknown type handling branch, you useformat!("{t:?}"). Verify that variabletis in scope and contains useful type information. Consider adding a comment explaining what is expected from t for unsupported types. - 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.
36. engine/language_client_python/src/errors.rs:19
- Draft comment:
When importing and calling functions on 'baml_py.internal_monkeypatch', using unwrap() can lead to panics if the module is missing. Consider adding error handling or fallback behavior. - 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.
37. engine/language_client_typescript/src/types/log_collector.rs:560
- Draft comment:
There is commented-out code in HTTPRequest.headers conversion. If this is intentional for future work, consider adding a TODO comment or clarifying how headers should be converted to JS objects. - 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.
38. engine/language_client_typescript/src/lib.rs:30
- Draft comment:
Initialization of the logger in module_init logs errors to stderr. Depending on your error handling strategy, you might prefer propagating these errors rather than silently continuing. - 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%
This is a module_init function called during Node.js module initialization. Propagating errors here would crash the entire module initialization. The current approach of logging and continuing is likely intentional since logging setup is not critical to module functionality. The comment suggests an alternative but doesn't make a strong case for why it would be better.
The comment raises a valid point about error handling patterns. Maybe there are cases where failing to initialize logging should be treated as a fatal error.
In a Node.js module init context, crashing on logging setup failure would be overly aggressive. The current pattern of graceful degradation is appropriate.
Delete the comment. While it raises an interesting point about error handling, the current approach is reasonable for this context and the comment doesn't make a strong enough case for change.
39. integ-tests/python/tests/test_collector.py:112
- Draft comment:
The tests rely on Collector.__function_span_count() to track function references. Ensure that tests running in parallel (or in different environments) correctly isolate global state; the usage of the serial_test decorator is appropriate but monitor for potential flakiness. - 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.
40. engine/baml-lib/jinja-runtime/src/lib.rs:1961
- Draft comment:
Typo found: In the comment block near line 1961, it says "we don't havea way". It would be clearer if it said "we don't have a way". - 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.
41. engine/baml-runtime/Cargo.toml:10
- Draft comment:
There's a potential typographical error: the key 'RSTEST_TIMEOUT' in the [package.metadata.rustflags] section may be intended to be 'RUST_TEST_TIMEOUT'. Please double-check whether this key is correctly spelled. - 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.
42. engine/baml-runtime/src/cli/init.rs:186
- Draft comment:
Minor typographical suggestion: In the comments beginning around line 186 (and repeated in similar blocks later), consider changing "auto generate libraries" to "auto-generated libraries" for improved readability. - 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.
43. engine/baml-runtime/src/tracingv2/publisher/publisher.rs:147
- Draft comment:
Typographical errors found in the comment. Please change "doesnt" to "doesn't" and "noone" to "no one" for better clarity. - 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.
44. engine/language_client_codegen/src/lib.rs:71
- Draft comment:
Typographical error: please change 'whhich' to 'which' in the comment on line 71. - 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.
45. engine/language_client_python/src/parse_py_type.rs:101
- Draft comment:
Typographical error: Consider renaming the variable 'infered' to 'inferred' for correct spelling. - 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.
46. engine/language_client_python/src/types/log_collector.rs:274
- Draft comment:
There's an extra '>' at the end of the repr string in LLMCall. Consider removing it for consistency with repr in LLMStreamCall. - 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.
47. engine/language_client_typescript/src/types/span.rs:39
- Draft comment:
Typo: The comment on line 39 reads "// mthod to finish". Please correct it to "// method to finish". - 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_TM7Ra2BFn9IXvZly
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 69d20bc in 2 minutes and 25 seconds
More details
- Looked at
484lines of code in22files - Skipped
0files when reviewing. - Skipped posting
17drafted comments based on config settings.
1. engine/baml-lib/baml-log/src/logger.rs:340
- Draft comment:
Changed log output text includes an emoji. Ensure that this change is intentional and does not break log parsing/consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the author to ensure that the change is intentional and does not break log parsing/consistency. This is a request for confirmation, which violates the rules. However, the comment does point out a potential issue with log parsing due to the emoji, which could be useful information for the author to consider. The comment could be rephrased to suggest checking log parsing without asking for confirmation.
2. engine/baml-lib/baml-log/src/logger.rs:326
- Draft comment:
New get_log_level function reads config with read lock. Confirm that error case returning DEFAULT_LOG_LEVEL is acceptable. - 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 asking for confirmation about a design choice, but looking at the codebase, DEFAULT_LOG_LEVEL is the standard fallback used everywhere. The error case here is consistent with the codebase's error handling patterns. The comment is asking for confirmation without providing evidence of an actual issue.
Perhaps there could be cases where returning DEFAULT_LOG_LEVEL masks serious lock issues that should be handled differently?
The lock error here is non-fatal and temporary - returning a default log level is a reasonable fallback that allows the system to continue operating, consistent with the rest of the codebase's error handling.
This comment should be deleted as it's asking for confirmation without identifying a concrete issue, and the code follows established patterns in the codebase.
3. engine/language_client_codegen/src/python/templates/globals.py.j2:5
- Draft comment:
Removed 'set_log_level' import from baml_py.baml_py. Confirm that removal doesn’t break external dependencies needing to configure logging via Python. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the removal of an import doesn't break external dependencies. This falls under the rule of not asking the author to confirm or ensure behavior. The comment doesn't provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. engine/language_client_python/python_src/baml_py/baml_py.pyi:9
- Draft comment:
Adjusted set_log_level log level literals (now uppercase) in Python interface. Please ensure this change is consistent with expected case usage in client code. - 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 consistency with expected case usage in client code, which violates the rule against asking the author to ensure behavior is intended. It does not provide a specific suggestion or ask for a specific test to be written.
5. engine/language_client_typescript/typescript_src/stream.ts:48
- Draft comment:
Polling for events with shift() and waiting 100ms in async iterator may be improved for efficiency. Consider using a more reactive pattern if high throughput is expected. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. integ-tests/python/tests/test_logger.py:28
- Draft comment:
Test case captures stdout to validate logging side effects. Verify that direct stdout prints (e.g., from Rust’s println!) are properly captured across environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that direct stdout prints are properly captured across environments. This falls under the category of asking the author to ensure or verify something, which is against the rules.
7. engine/baml-lib/baml-log/src/lib.rs:48
- Draft comment:
Re-export now includes get_log_level. Ensure the ordering is intentional and documented, so consumers know the new API ordering. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. engine/baml-lib/baml-log/src/logger.rs:340
- Draft comment:
The log message now includes a sheep emoji ([BAML 🐑]). Ensure that the emoji is acceptable in production and that its behavior is documented for users with non-ASCII terminals. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the author to ensure that the emoji is acceptable in production and that its behavior is documented. This falls under asking the author to ensure behavior, which is against the rules. However, the presence of an emoji in logs could be a real issue if it affects readability or compatibility, so it might be useful to point out potential issues with non-ASCII terminals.
9. engine/language_client_codegen/src/python/templates/globals.py.j2:5
- Draft comment:
set_baml_log_level function was removed from the Python globals template. Confirm that this API removal is intentional and update documentation if users relied on it. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment is asking the author to confirm the intention behind the removal of a function, which is against the rules. However, it also suggests updating documentation if users relied on it, which is a specific action. The comment is partially useful but mostly violates the rules.
10. engine/language_client_python/python_src/baml_py/baml_py.pyi:9
- Draft comment:
Signature for set_log_level now accepts uppercase log levels (DEBUG, INFO, WARN, etc.). This is a breaking change compared to the lowercase versions. Make sure this change is clearly documented for Python users. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is informative and suggests ensuring documentation for a breaking change. It doesn't ask for confirmation or testing, but it does suggest a documentation update, which is not allowed by the rules.
11. engine/language_client_typescript/native.js:392
- Draft comment:
Native bindings now export getLogLevel, setLogLevel, and setLogJson. Verify that the TypeScript definitions and usage are consistent across all clients. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. engine/language_client_typescript/typescript_src/stream.ts:18
- Draft comment:
Updated error conversion from asBamlError to toBamlError. Confirm that this new function produces the expected error payload and does not hide useful debug information. - 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. integ-tests/python/tests/test_logger.py:26
- Draft comment:
The integration tests capture stdout and assert that it remains empty. Verify that printing from set_log_level (which now prints a message when level changes) does not interfere with these tests, or consider isolating log messages from function output. - Reason this comment was not posted:
Comment looked like it was already resolved.
14. integ-tests/typescript/tests/logger.test.ts:22
- Draft comment:
The TypeScript tests capture stdout to ensure no unintended output is printed. Reassess if the new logging output ([BAML 🐑] ...) might appear in future changes or when log level changes occur. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
This comment is informative and suggests reassessing the impact of new logging output on tests. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't confirm if the change is intentional or suggest a best practice. Therefore, it seems to violate the rules.
15. engine/baml-lib/baml-log/src/logger.rs:443
- Draft comment:
Typographical error: An extraneous colon appears after the parameter list in the definition oflog_event_internal(line 443). Please remove the colon to ensure proper Rust syntax. - 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. engine/language_client_typescript/native.d.ts:216
- Draft comment:
Typographical error: The declaration for get_version has a duplicate 'export declare'. Please remove the extra occurrence. - Reason this comment was not posted:
Comment was on unchanged code.
17. engine/language_client_typescript/native.d.ts:220
- Draft comment:
Typographical error: The declaration for invoke_runtime_cli has 'export declare' written twice. Please remove the redundant part. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_yQTOa1989M6Ik4rv
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.
| @@ -215,6 +215,8 @@ export interface BamlLogEvent { | |||
|
|
|||
| export declare export declare function get_version(): string | |||
|
|
|||
| export declare export declare function getLogLevel(): string | |||
There was a problem hiding this comment.
Typographical error: The declaration for getLogLevel has a duplicated export declare. Please fix by removing the duplicate.
Suggested change
| export declare export declare function getLogLevel(): string | |
| export declare function getLogLevel(): string |
| @@ -223,3 +225,7 @@ export interface LogEventMetadata { | |||
| rootEventId: string | |||
| } | |||
|
|
|||
| export declare export declare function setLogJson(useJson: boolean): void | |||
There was a problem hiding this comment.
Typographical error: The declaration for setLogJson contains a duplicate export declare. Please remove the extra instance.
Suggested change
| export declare export declare function setLogJson(useJson: boolean): void | |
| export declare function setLogJson(useJson: boolean): void |
| @@ -223,3 +225,7 @@ export interface LogEventMetadata { | |||
| rootEventId: string | |||
| } | |||
|
|
|||
| export declare export declare function setLogJson(useJson: boolean): void | |||
|
|
|||
| export declare export declare function setLogLevel(level: string): void | |||
There was a problem hiding this comment.
Typographical error: The declaration for setLogLevel has a duplicated export declare. Please remove the duplicate.
Suggested change
| export declare export declare function setLogLevel(level: string): void | |
| export declare function setLogLevel(level: string): void |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 06e2f30 in 2 minutes and 17 seconds
More details
- Looked at
205lines of code in10files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. engine/language_client_codegen/src/python/templates/__init__.py.j2:40
- Draft comment:
Added 'logging' to the all list appears intentional. Ensure documentation and usage are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. engine/language_client_codegen/src/python/templates/logging.py.j2:1
- Draft comment:
New logging module correctly re-exports set_log_level and get_log_level. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. engine/language_client_codegen/src/typescript/templates/index.ts.j2:44
- Draft comment:
Consistent re-export of the logging module via 'export * as logging' looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. fern/03-reference/baml_client/logging.mdx:22
- Draft comment:
Documentation correctly shows usage of set_log_level. Verify that Ruby and other language stubs are updated when implemented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. integ-tests/python/baml_client/globals.py:40
- Draft comment:
Removal of 'set_baml_log_level' is intentional; ensure that any documentation and client code now references the logging module directly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. integ-tests/python/tests/test_logger.py:10
- Draft comment:
The reset_log_level fixture uses 'logging.get_log_level()', but 'logging' is not imported in this file. Use the directly imported 'get_log_level()' and 'set_log_level()' instead. - Reason this comment was not posted:
Marked as duplicate.
7. fern/docs.yml:417
- Draft comment:
The new page entry uses 'logging / env vars' with lowercase letters and spaces around the '/'. For consistency with other page titles (like 'TypeBuilder' and 'ClientRegistry'), consider adjusting the capitalization and possibly removing the extra spaces (e.g., 'Logging/Env Vars' or 'Logging / Env Vars'). This will ensure a uniform style throughout the navigation. - 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%
While the comment points out an inconsistency, this appears to be a documentation site where readability for users is more important than strict formatting consistency. The current format "logging / env vars" is clear and readable. The surrounding entries that use PascalCase are actual code/class names (TypeBuilder, ClientRegistry), so they follow a different convention for a good reason. This isn't a clear case where standardization would improve things.
The comment assumes that consistency with code identifiers should apply to documentation page titles, which may not be true. Documentation often benefits from being more human-readable.
Agreed - documentation navigation should prioritize clarity and readability over strict formatting rules. The current format is clear and appropriate for a documentation page title.
The comment should be deleted as it suggests an unnecessary change that could make the documentation navigation less readable. The current format is appropriate for its purpose.
8. integ-tests/python/tests/test_logger.py:10
- Draft comment:
Typographical error: In the fixture reset_log_level, 'logging.get_log_level()' is used but 'logging' is not imported. It should be just 'get_log_level()', matching the imported function from '..baml_client.logging'. - Reason this comment was not posted:
Marked as duplicate.
9. integ-tests/python/tests/test_logger.py:12
- Draft comment:
Typographical error: In the fixture reset_log_level, 'logging.set_log_level(previous_level)' is used but 'logging' is not imported. It should be 'set_log_level(previous_level)' in order to use the function imported from '..baml_client.logging'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_wvkn41ozakzXY6hl
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.
|
|
||
| @pytest.fixture(scope="function") | ||
| def reset_log_level(): | ||
| previous_level = logging.get_log_level() |
There was a problem hiding this comment.
Use the imported get_log_level() instead of logging.get_log_level() since logging is not defined in this scope.
hellovai
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cc1fb1f in 2 minutes and 27 seconds
More details
- Looked at
2948lines of code in61files - Skipped
0files when reviewing. - Skipped posting
22drafted comments based on config settings.
1. integ-tests/typescript/tests/logger.test.ts:1
- Draft comment:
Logger test file is clear and well‐structured. The helper function captureStdout using jest.spyOn correctly captures process.stdout output, and the tests properly save and restore the log level state. No changes required. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. integ-tests/typescript/tests/logger.test.ts:22
- Draft comment:
The test file is well-structured and clearly documents its purpose. The helper function using jest.spyOn to capture stdout is implemented appropriately. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. integ-tests/typescript/tests/logger.test.ts:42
- Draft comment:
Tests correctly set and assert log levels. Consider whether strict equality with an empty string (expect(output).toBe('')) is robust enough if minor whitespace differences could occur; using .trim() might add resilience. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. engine/baml-runtime/src/internal/llm_client/mod.rs:65
- Draft comment:
Typo in comment: "Node that we set the StreamState..." should be "Note that we set the StreamState...". - 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/language_client_codegen/src/typescript/mod.rs:244
- Draft comment:
Typo: 'typscript_client' should be 'typescript_client' for consistency. - 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. engine/language_client_codegen/src/typescript/mod.rs:254
- Draft comment:
Typo: Rename 'typscript_client' to 'typescript_client' to match naming conventions. - 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. engine/language_client_codegen/src/typescript/mod.rs:363
- Draft comment:
Typo: 'typscript_client' should be 'typescript_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.
8. engine/language_client_codegen/src/typescript/mod.rs:371
- Draft comment:
Typo: 'typscript_client' should be corrected to 'typescript_client' for clarity. - 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. engine/language_client_codegen/src/typescript/mod.rs:390
- Draft comment:
Typo: Replace 'typscript_client' with 'typescript_client' to maintain consistency. - 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. engine/language_client_typescript/native.d.ts:216
- Draft comment:
There are duplicate 'export declare' tokens (e.g., on lines 216, 218, 220, 228, 230, and 232). Please remove the extra occurrences so that only one 'export declare' remains before each function declaration. - 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. engine/language_client_typescript/native.d.ts:133
- Draft comment:
The 'toString()' method is declared twice in the LlmCall class (lines 133 and 134). Please remove the duplicate declaration. - 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. engine/language_client_typescript/native.d.ts:139
- Draft comment:
The 'toString()' method is declared twice in the LlmStreamCall class (lines 139 and 147). Please remove the duplicate declaration. - 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. fern/03-reference/baml_client/config.mdx:14
- Draft comment:
Typographical suggestion: On line 14, consider changing "Setting can also be modified via functions inbaml_client.config." to "Settings can also be modified via functions inbaml_client.config." for consistency with the rest of the document. - 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%
While the suggestion is technically correct and would improve consistency, PR comments about minor typos or grammar are generally not important enough to warrant a comment. These kinds of small issues can be fixed directly or mentioned in a general review comment rather than as inline comments.
The change would improve document consistency and readability. Grammar errors in documentation could reflect poorly on the project's quality.
While correct, this is an extremely minor issue that doesn't impact understanding. PR comments should focus on more substantial issues.
Delete this comment as it's too minor of an issue to warrant a standalone PR comment.
14. fern/docs.yml:477
- Draft comment:
Typo detected: In the page title "openai-generic: Azure AI Foundary", 'Foundary' seems to be misspelled. Consider correcting it to 'Foundry' if that is the intended term. - 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. integ-tests/openapi/baml_client/openapi.yaml:2328
- Draft comment:
There are $ref references for the 'Check' schema that are missing the leading '/' after the '#' (e.g. '$ref: "#components/schemas/Check"' should be '$ref: "#/components/schemas/Check"'). Please update these to maintain consistency and ensure correct resolution. - 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. integ-tests/openapi/baml_client/openapi.yaml:2866
- Draft comment:
In the 'ExtractReceiptInfo' request body, the 'reason' property uses a 'oneOf' list with duplicate type definitions (both are 'type: string'). This redundancy could be simplified to a single entry. Please review and update as necessary. - 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.
17. integ-tests/react/baml_client/react/server_streaming.ts:1394
- Draft comment:
In the union type for parameter 'c' in the NestedAlias function, 'number' is repeated; please remove the duplicate to clean up the type definition. - 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.
18. integ-tests/react/baml_client/react/server_streaming.ts:1515
- Draft comment:
In the union type for parameter 'p' in the PrimitiveAlias function, 'number' appears twice. Please remove the duplicate to ensure the type is defined correctly. - 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.
19. integ-tests/react/baml_client/react/server_streaming_types.ts:24
- Draft comment:
Minor spacing issue: In the import statement on line 24, there's no space between the closing brace '}' and the 'from' keyword. Consider changingimport type { partial_types }from "../partial_types";toimport type { partial_types } from "../partial_types";for better readability. - Reason this comment was not posted:
Comment was on unchanged code.
20. integ-tests/react/baml_client/sync_client.ts:61
- Draft comment:
Typographical: The error message in the 'stream' getter is missing a closing backtick (and possibly additional punctuation). Consider revising it to something like: "stream is not available in BamlSyncClient. Useimport { b } from 'baml_client/async_client" so that the inline code formatting is properly closed. - 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.
21. integ-tests/react/baml_client/sync_client.ts:57
- Draft comment:
Typographical: In the deprecation comment for the stream getter, the phrase 'streaming must by async' appears. This likely should read 'streaming must be async'. - 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.
22. integ-tests/typescript/baml_client/config.ts:19
- Draft comment:
There's no newline character at the end of the file. Please add a newline at the end for consistency and to meet common formatting standards. - 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%
Since this is an auto-generated file (clearly stated in comments), suggesting manual edits to it doesn't make sense. The file format should be controlled by the generation process, not by manual edits. Additionally, missing newline at EOF is something that would typically be caught by linters or formatters if it was important to the project.
The missing newline could cause issues with some tools or make git diffs less clean. It's a widely accepted best practice to have newlines at EOF.
While newline at EOF is good practice, this file is auto-generated. Any formatting issues should be fixed in the generator, not through manual edits which would be overwritten anyway.
Delete this comment because it suggests manually editing an auto-generated file, which is explicitly discouraged in the file's header comments.
Workflow ID: wflow_2Ra3i3z9VZxkK9kh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Introduces
baml-logfor structured logging, integrates it across the codebase, and adds tests for logging behavior.baml-logfor structured logging with JSON and standard formats.BAML_LOGandBAML_LOG_JSON.baml-loginbaml-runtime,cli, and language client libraries.dev.rs,generate.rs,init.rs,serve/mod.rs, and other files.logger.test.tsto test logging behavior with different log levels.Cargo.lockandCargo.tomlto includebaml-logdependencies.This description was created by
for cc1fb1f. It will automatically update as commits are pushed.