Task/2 3 2025 json schema for filters#1663
Conversation
… format. Also adding additional validator constructs needed to complete this.
…2025-json-schema-for-filters
|
this approach looks nice and for the current cases it looks like less code to define the params. Q: How are surprises handled, and how does that compare to our older error messages? |
…2025-json-schema-for-filters
…2025-json-schema-for-filters
cyrush
left a comment
cyrush
left a comment
There was a problem hiding this comment.
Looks great a few inline comments.
Also, we should consider accepting snake_case schema specifiers along with camelCase ones -- not sure it is worth the effort but would look nicer
src/libs/ascent/runtimes/expressions/ascent_expression_jit_filters.cpp
Outdated
Show resolved
Hide resolved
| param_schema["properties/sphere"].set(sphere_schema); | ||
|
|
||
| // --- cylinder --- | ||
| conduit::Node cylinder_schema; |
There was a problem hiding this comment.
consider using a ref to node in the tree instead of a separate node.
There was a problem hiding this comment.
Updated everything to use refrences to nodes rather than new seperate nodes. I also updated the docs in #1682 to reflect these changes
|
|
||
| using ExpressionCheckFn = bool (*)(const std::string &expr, std::string &err_msg); | ||
|
|
||
| void FLOW_API set_expression_checker(ExpressionCheckFn fn); |
There was a problem hiding this comment.
json schema has a concept of string format checkers.
Generalize such that we can register format callbacks for categories of format checkers.
register_format_checker(const std::string &case, callback)
and then when we use in ascent we can
register_format_checker("expression",our_ascent_callback)
| // ---------- General Helpers ---------- | ||
| ExpressionCheckFn &expr_checker() | ||
| { | ||
| static ExpressionCheckFn fn = nullptr; |
There was a problem hiding this comment.
this can become a map of format checkers to support general cases
| const auto data_type = input.dtype(); | ||
| bool ok = true; | ||
|
|
||
| if(schema_defined_type == "object") ok = data_type.is_object(); |
There was a problem hiding this comment.
formatting suggestion, final case uses {}, it would be good for even the oneliners to use
{
oneliner
}
There was a problem hiding this comment.
I went through and fixed as many of these as I could find
| const std::string k = input[i].name(); | ||
| if(!has_props || !props.has_child(k)) | ||
| { | ||
| add_error(info, "Unexpected field '" + conduit::utils::join_file_path(path, k) + |
There was a problem hiding this comment.
You can use join_path, for conduit path, vs join_file_path for filesystem paths
| add_error(info, "Unexpected field '" + conduit::utils::join_file_path(path, k) + | |
| add_error(info, "Unexpected field '" + conduit::utils::join_path(path, k) + |
There was a problem hiding this comment.
Change applied locally in a commit (on the branch #1682 whoops)
| if(!schema["format"].dtype().is_string()) return true; | ||
|
|
||
| const std::string fmt = schema["format"].as_string(); | ||
| if(fmt != "expression") return true; |
There was a problem hiding this comment.
this is were we would implement general format callbacks
|
@cyrush I went through and addressed all of your feedback. on all three branches. I tried to keep things still relatively with the original changes to keep things less noisy. I did update the documentation in #1682 to reflect the changes that I made so that might be a good place to start reviewing from. |
2 participants
This is just a first pass and very much a draft version
Most focus was placed on building the actual structure. Before merging into an official version significant revisions to error handling in the validator should be made to add clarity.
This schema has currently only been added in
ascent_runtime_vtkh_filters. The goal is to implement this for all filters in Ascent. Once this has been done and the full scope of Json Schema features needed has been determined, theflow_schema_validatorshould be migrated to conduit.