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

feat: Add constraints via Jinja expressions #1006

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Oct 2, 2024

Important

This PR adds support for constraints using Jinja expressions, updates models and validation logic, and enhances testing for constraint functionality.

  • Behavior:
    • Add support for constraints using Jinja expressions in baml-core.
    • Implement evaluate_predicate in internal_baml_jinja for constraint evaluation.
    • Update FieldType to support Constrained type with constraints.
  • Models:
    • Add Constraint and ConstraintLevel in baml-types.
    • Update NodeAttributes to include constraints.
  • Validation:
    • Add validation for constraints in validation_pipeline.
    • Ensure constraints are not allowed as function parameters.
  • Testing:
    • Add tests for constraints in test_constraints.rs.
    • Update integration tests to check constraint functionality.
  • Misc:
    • Add itertools dependency in Cargo.lock.
    • Update shell.nix to include necessary tools for development.

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

Copy link

vercel bot commented Oct 2, 2024

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 Oct 23, 2024 11:34pm

@imalsogreg imalsogreg force-pushed the greg/constraints branch 2 times, most recently from bfcb239 to 0e483ac Compare October 7, 2024 23:07
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 4ad7ea4 in 2 minutes and 12 seconds

More details
  • Looked at 70 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. engine/baml-lib/baml-types/src/baml_value.rs:464
  • Draft comment:
    Use iter() instead of into_iter() to avoid consuming the vector.
                v.iter().map(|v| v.map_meta(f)).collect(), f(m)
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. engine/baml-lib/baml-types/src/baml_value.rs:470
  • Draft comment:
    Use iter() instead of into_iter() to avoid consuming the vector.
                fs.iter().map(|(k, v)| (k.clone(), v.map_meta(f))).collect(), f(m)
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_GmiEyLxH0oXTxeSp


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.

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 362a80f in 1 minute and 51 seconds

More details
  • Looked at 34 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_deQM19aeYuQFSYbi


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 a11d96e in 1 minute and 55 seconds

More details
  • Looked at 29 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. shell.nix:43
  • Draft comment:
    Ensure that lld_18 is necessary for the build environment and compatible with other tools.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of lld_18 in shell.nix should be verified for necessity and compatibility with the rest of the environment.

Workflow ID: wflow_BULDaaVB6hGS0TG0


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.

integ-tests/python/tests/test_functions.py Outdated Show resolved Hide resolved
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 a1ade48 in 1 minute and 22 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-runtime/src/tracing/mod.rs:8
  • Draft comment:
    Remove unused import internal_baml_core::ir::jinja_helpers::ConstraintExpressionError.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The unused import should be removed to maintain clean code.

Workflow ID: wflow_PLiXmR5YtAHijy5R


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 0f18524 in 1 minute and 17 seconds

More details
  • Looked at 319 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-runtime/src/cli/serve/mod.rs:366
  • Draft comment:
    Consider refactoring the repeated pattern of handling result_with_constraints_content() into a helper function to reduce code duplication and improve maintainability. This pattern appears in multiple files, such as engine/baml-runtime/src/tracing/mod.rs, engine/baml-runtime/src/types/response.rs, and others.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in multiple files has a repeated pattern of handling the result of result_with_constraints_content(). This pattern can be simplified by using a helper function to avoid code duplication and improve maintainability.

Workflow ID: wflow_d0nh26cIIfIFDyLB


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 ac8b7ed in 1 minute and 11 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/baml-types/src/baml_value.rs:446
  • Draft comment:
    Consider re-adding the unimplemented!() macro to handle potential future variants of BamlValue. This ensures that any new variants are explicitly handled.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_5rQ3a6xbhrwhlzQz


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 7508237 in 2 minutes and 39 seconds

More details
  • Looked at 9870 lines of code in 129 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:185
  • Draft comment:
    Consider using anyhow::anyhow! consistently for error handling instead of mixing with anyhow::bail!.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of anyhow::bail! and anyhow::anyhow! for error handling is inconsistent. It's better to use one consistently for clarity and maintainability.
2. engine/baml-lib/baml-core/src/ir/jinja_helpers.rs:87
  • Draft comment:
    Consider providing a more informative error message for the evaluate_predicate function. This will help in debugging issues related to constraint evaluation.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message for the evaluate_predicate function is not informative. It should provide more context about the error.

Workflow ID: wflow_qOhG8h1Q2Zeu1rZT


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 7f2b597 in 1 minute and 15 seconds

More details
  • Looked at 72 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/jsonish/src/tests/mod.rs:123
  • Draft comment:
    The function distribute_constraints is used here but has been removed. This will cause a compilation error. Consider re-implementing or replacing its functionality.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment correctly identifies that the function distribute_constraints was removed and that the code now uses output.distribute_constraints(). This suggests a method call on output, which is likely an instance of a type that now has distribute_constraints as a method. The comment is valid because it highlights a potential issue if the method is not correctly implemented, leading to a compilation error.
    I might be assuming too much about the context of the change without seeing the implementation of distribute_constraints as a method. It's possible that the method is correctly implemented elsewhere, and the comment might be unnecessary.
    The comment is still useful because it highlights a potential issue that needs to be verified, even if the method is implemented elsewhere. It ensures that the change is intentional and correctly handled.
    Keep the comment as it correctly identifies a potential issue with the removal of the function and the change to a method call, which could lead to a compilation error if not handled properly.

Workflow ID: wflow_Bupm5BgMOgBKUHPo


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 dfaad86 in 1 minute and 10 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/attributes/to_string_attribute.rs:30
  • Draft comment:
    The comment indicates that the validation logic for assert and check attributes is not executed. This could lead to unexpected behavior if these attributes are used. Consider investigating why this validation is not triggered and ensure it is executed as intended.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_JJJCcBOVBE0O4mQU


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 6aac2cd in 1 minute and 18 seconds

More details
  • Looked at 47 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_n8JHIwQa5MJahoRW


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 aa31f42 in 1 minute and 30 seconds

More details
  • Looked at 67 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/language_client_codegen/src/typescript/mod.rs:12
  • Draft comment:
    The check_types field was removed from AsyncTypescriptClient, SyncTypescriptClient, and TypescriptClient. Ensure that any functionality relying on type checking is not affected by this removal.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_uZSAu47dAOmlbGhp


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 316ac5f in 2 minutes and 12 seconds

More details
  • Looked at 37 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/language_client_ruby/ext/ruby_ffi/src/ruby_to_json.rs:30
  • Draft comment:
    The type_name_for_checks function returns a hardcoded "String". Consider implementing it properly or removing it if not needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about a function that was removed in the diff, so the issue it raises has already been resolved. Therefore, the comment is not necessary.
    I might be missing the context of why the function was removed, but since the function is no longer present, the comment is not relevant.
    The removal of the function indicates that the issue raised by the comment has been addressed, making the comment obsolete.
    The comment should be deleted because the function it refers to has been removed, resolving the issue.

Workflow ID: wflow_PXbzRrNmK6uD0Jys


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

Copy link
Contributor

ellipsis-dev bot commented Oct 23, 2024

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

Copy link
Contributor

ellipsis-dev bot commented Oct 23, 2024

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

@imalsogreg imalsogreg added this pull request to the merge queue Oct 23, 2024
Merged via the queue into canary with commit d794f28 Oct 23, 2024
10 checks passed
@imalsogreg imalsogreg deleted the greg/constraints branch October 23, 2024 23:36
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