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: implement maps in BAML #797

Merged
merged 11 commits into from
Jul 19, 2024
Merged

feat: implement maps in BAML #797

merged 11 commits into from
Jul 19, 2024

Conversation

sxlijin
Copy link
Collaborator

@sxlijin sxlijin commented Jul 17, 2024

Add support for maps in the BAML language. We currently only support maps with string keys.

Here's the work item list that we verified (fortunately, most of these were already pre-implemented):

  • integ-tests (python, ts, ruby)
  • parser for the compiler
    • grammar
    • schema-ast
  • parser for codemirror
    • lezer
  • vscode: textmate grammar
  • diagnostics
    • not sure what this means concretely
  • in-Rust implementation, i.e. BamlValue
  • value syntax
    • ie how you define a map in a test
  • input layer (lang-native object → BamlValue)
  • ctx.output_format
  • output layer (BamlValue → lang-native object)
  • codegen (python, ts, ruby)

We also added:

  • a new entry point for validate_type_allowed where we apply refinements to the lexed types
  • textmate grammar snapshot testing using vscode-tmgrammar-snap

Fixes #787

Copy link

vercel bot commented Jul 17, 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 Jul 19, 2024 5:39pm

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

  • Added support for maps with string keys in BAML language
  • Implemented coerce_map function in engine/baml-lib/jsonish/src/deserializer/coercer/coerce_map.rs
  • Updated docs/docs/snippets/supported-types.mdx with map syntax and usage
  • Added new test files for map types in integ-tests/baml_src/test-files/functions/input/named-args/single/
  • Modified engine/baml-lib/jsonish/src/deserializer/score.rs to handle map parsing errors

18 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

Comment on lines 9 to 10
a2 map<string, int>
a2 map<string, MapDummy>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Duplicate field name 'a2'. Rename one of these fields.

Comment on lines 38 to 39
// TODO(vbv): this seems like a bug - it causes more penalization the deeper into an array it is?
Err(e) => flags.add_flag(Flag::ArrayItemParseError(i, e)),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Consider addressing this potential bug in error penalization for nested arrays

Comment on lines 15 to 19
myMap {
"key" "example string"
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Test case doesn't match function parameter type. Should be StringToClassEntry object, not string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait isnt greptile correct here?

myMap {
"key" { word "example string" }
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Updated docs/docs/snippets/supported-types.mdx: Maps now only support string keys
  • Modified validation in engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/*.rs
  • Added engine/baml-lib/baml/tests/validation_files/class/map_types.baml for map type testing
  • Introduced engine/baml-lib/jsonish/src/tests/test_maps.rs for map deserialization tests
  • Updated error messages in engine/baml-lib/baml/tests/validation_files/class/secure_types.baml

9 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Added support for maps in BAML language
  • Implemented string-only keys for maps
  • Updated validation for map types in engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/*.rs
  • Added map type testing in engine/baml-lib/baml/tests/validation_files/class/map_types.baml
  • Introduced map deserialization tests in engine/baml-lib/jsonish/src/tests/test_maps.rs

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

No major changes found since last review.

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@BoundaryML BoundaryML deleted a comment from greptile-apps bot Jul 18, 2024
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Implemented map types with string keys in BAML language
  • Updated secure_types.baml to use new map<> syntax
  • Replaced curly braces {} with map<> for defining map types
  • Added new entry point for validate_type_allowed with refinements
  • Introduced textmate grammar snapshot testing using vscode-tmgrammar-snap

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -0,0 +1,45 @@
use super::*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sxlijin some more tests would be nice

map with new lines, for keys,

partial maps
nested partial maps
maps with ints as keys to ensure they cast to strings
maps with arrays as key to ensure they cast to strings
maps in json markup
nested maps with new lines
union of map vs class (would the map correctly let the class have it?)

@@ -17,207 +17,45 @@ class ComplexTypes {
o (((int | string) | bool[]), (float, double) | long_long_identifier_123)
}

// error: Type `apple_pie` does not exist. Did you mean one of these: `ComplexTypes`, `float`, `bool`, `string`, `int`?
// error: Error validating: This line is not a valid field or attribute definition. A valid class property looks like: 'myProperty string[] @description("This is a description")'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error got much worse.

seems we broke the grammar :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you reviewed an outdated version of the PR - the grammar wasn't broken, the test was still using the {key: value} map syntax

Copy link
Contributor

@hellovai hellovai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost, just one grammar bug it seems.

also what does the prompt look like for a map type? I don't see how the code changes for that

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Updated .envrc to include LLVM binaries in PATH
  • Added new test cases for map types in engine/baml-lib/baml/tests/validation_files/class/map_types.baml
  • Updated dependencies in engine/baml-lib/jinja/Cargo.toml to support map functionality
  • Introduced map_style keyword in engine/baml-lib/jinja/src/output_format/mod.rs
  • Added support for dictionary types in engine/language-client-codegen/src/python/templates/partial_types.py.j2

19 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

word string
}
// test string
function TestFnNamedArgsSingleMapStringToClass(myMap: int) -> map<string, StringToClassEntry> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Function parameter type changed from map<string, StringToClassEntry> to int, which is inconsistent with the expected map structure.

Comment on lines +16 to +19
"key" {
word "lorem ipsum"
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Test case updated to match the new parameter type, but this does not align with the function's intended purpose of handling a map.

@@ -159,6 +162,11 @@ class Person < T::Struct
const :name, T.nilable(String)
const :hair_color, T.nilable(Baml::Types::Color)
end
class Quantity < T::Struct
include T::Struct::ActsAsComparable
const :amount, T.nilable(T.any(T.nilable(Integer), T.nilable(Float)))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Consider validating that amount is either an Integer or Float to avoid potential type issues.

@@ -248,6 +248,12 @@ export interface Person {
[key: string]: any;
}

export interface Quantity {
amount: number | number
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: Duplicate type in amount: number | number. Should be number.

greptile-apps[bot]

This comment was marked as spam.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Updated .envrc to include LLVM binaries in PATH.
  • Added docs/package.json with fern-api dependency.
  • Introduced map_types.baml for map validation tests.
  • Modified Cargo.toml in baml-lib/jinja to add strum dependency.
  • Added coerce_map.rs for JSON to BAML map coercion.

35 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings

word string
}
// test string
function TestFnNamedArgsSingleMapStringToClass(myMap: int) -> map<string, StringToClassEntry> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Function parameter type changed from map<string, StringToClassEntry> to int, which is inconsistent with the expected map structure.

Comment on lines +16 to +19
"key" {
word "lorem ipsum"
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic: Test case updated to match the new parameter type, but this does not align with the function's intended purpose of handling a map.

@@ -248,6 +248,12 @@ export interface Person {
[key: string]: any;
}

export interface Quantity {
amount: number | number
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: Spelling: Duplicate type in amount: number | number. Should be number.

Suggested change
amount: number | number
amount: number

@sxlijin
Copy link
Collaborator Author

sxlijin commented Jul 19, 2024

todo: lex maps of arbitrary parity

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

  • Added engine/baml-lib/baml/tests/validation_files/class/map_types2.baml for additional map type validation tests.
  • Renamed tools/update_macos_llvm.sh to automate LLVM and Rust setup on macOS, ensuring WASM support.
  • Watch out for potential issues with Rust reinstallation affecting existing configurations.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@sxlijin sxlijin merged commit 97d7e62 into canary Jul 19, 2024
8 of 9 checks passed
@sxlijin sxlijin deleted the sam/map-docs branch July 19, 2024 17: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.

BAML language: maps
3 participants