Implement Annotation config types - #106
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a Rust-native annotation configuration model (serde/YAML compatible) plus validation logic and unit tests, wiring the new annotation module into the crate and CI.
Changes:
- Added serde-compatible annotation config types (
AnnotationType,AnnotationDelimiter,AnnotationFormat, and config structs) and re-exported them viasrc/annotation/mod.rs. - Implemented YAML parse + validation entrypoints (
parse_and_validate,validate_config) and added unit tests for config round-trips and validation scenarios. - Updated build tooling and CI to run Rust tests, added stricter clippy settings, and introduced
serde/yaml_serdedependencies.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/annotation/config.rs |
New serde/YAML config enums + structs for annotation configuration. |
src/annotation/mod.rs |
New module entrypoint and public re-exports for config + validator APIs. |
src/annotation/validator.rs |
New YAML parse + validation logic for annotation configs. |
src/lib.rs |
Exposes annotation module publicly and enables in-crate unit tests. |
src/tests/mod.rs |
Adds crate unit test module root. |
src/tests/test_annotation/mod.rs |
Adds annotation test module wiring. |
src/tests/test_annotation/test_config.rs |
Round-trip YAML serialization/deserialization tests for enums + config defaults. |
src/tests/test_annotation/test_validator.rs |
Tests for validator success and error cases. |
Cargo.toml |
Adds serde and yaml_serde dependencies needed for YAML config support. |
Cargo.lock |
Lockfile updates for new dependencies. |
Makefile |
Adds fmt/fix targets and makes clippy warnings fatal in lint/check. |
.github/workflows/openvariant_tester.yml |
Runs Rust tests in CI in addition to existing Python tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…lab/openvariant into 64-implement-annotation-config-types
FedericaBrando
left a comment
There was a problem hiding this comment.
Thanks David for the work - very fun to review!
I have added some comments to be address, given the conversation we had on Slack.
Some other points:
- We should check if the fileMapping exists and fail fast if the file does not exist, also we should allow absolute paths, not only relatives to the yaml
- We should enforce the function field to start with
lambda [var] : funct() - One of the main pain points in openvariant as of now is the lack of error message when a field fails. Here we could be more verbose about what is failing during validation
Below I attach a git diff patch to be applied with suggested changes. Feel free to remove, change, or discard them if you think this is not what you were thinking.
Thanks!
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] | ||
| pub enum AnnotationDelimiter { | ||
| /// Tab character (`\t`). | ||
| #[default] | ||
| T, | ||
| /// Comma character (`,`). | ||
| C, | ||
| } | ||
|
|
||
| impl AnnotationDelimiter { | ||
| /// Returns the delimiter as a `char`. | ||
| pub fn as_char(&self) -> char { | ||
| match self { | ||
| AnnotationDelimiter::T => '\t', | ||
| AnnotationDelimiter::C => ',', | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for AnnotationDelimiter { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let s = match self { | ||
| AnnotationDelimiter::T => "T", | ||
| AnnotationDelimiter::C => "C", | ||
| }; | ||
| write!(f, "{s}") | ||
| } | ||
| } |
There was a problem hiding this comment.
I remember we added a feature where the delimiter could be detected automatically if the user did not set it - how this implementation include also the above mentioned case? Given that we are given as delimiter T or C and we default to T, if none is provided? #52
| /// AnnotationFormat | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] | ||
| pub enum AnnotationFormat { |
There was a problem hiding this comment.
can't we use:
#[serde(rename_all = "UPPERCASE")]| /// AnnotationEntry | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| pub struct AnnotationEntry { |
There was a problem hiding this comment.
Here we are using a struct, althugh serde does implement a tagged enum that maybe is useful in our case, and enforces the annotation field to have the correct entries.
#[serde(tag = "type", rename_all = "lowercase")]
enum AnnotationEntry {
Static {
field: String,
value: yaml_serde::Value,
},
Internal {
field: String,
#[serde(default)]
value: Option<String>,
field_source: <Vec<String>>,
#[serde(default)]
function: Option<String>,
},
Filename {
field: String,
#[serde(default)]
function: Option<String>,
#[serde(default)]
regex: Option<String>,
},
Dirname {
field: String,
#[serde(default)]
function: Option<String>,
#[serde(default)]
regex: Option<String>,
},
Plugin {
field: String,
plugin: String,
},
Mapping {
field: String,
field_source: Vec<String>,
field_mapping: String,
file_mapping: String,
field_value: String,
},
}| // Plugin | ||
| AnnotationType::Plugin => { | ||
| check_required_str(&base, "plugin", entry.plugin.as_deref(), &mut diags); | ||
| check_required_str(&base, "function", entry.function.as_deref(), &mut diags); |
There was a problem hiding this comment.
function is not a field of Plugin, it is in fact a field of filename, dirname or internal right?
| /// | ||
| /// Returns *all* diagnostics (errors and warnings) found. | ||
| /// An empty `Vec<ValidationError>` means the configuration is fully valid, no errors found. | ||
| pub fn validate_config(config: &AnnotationConfig) -> Vec<ValidationError> { |
There was a problem hiding this comment.
if we use serde tagged enums, then we could reduce the fields that are validated. For example only validate non duplicated field within a list, if the values are non-empty and all the things that you can't validate with structure.
| /// included for context). | ||
| pub fn parse_and_validate(yaml: &str) -> Result<AnnotationConfig, Vec<ValidationError>> { | ||
| // Pass 1 — syntax + structural (serde) | ||
| let config: AnnotationConfig = yaml_serde::from_str(yaml).map_err(|e| { |
There was a problem hiding this comment.
Here couldn't we read it from a buffer directly?
https://docs.rs/yaml_serde/latest/yaml_serde/fn.from_reader.html
This pull request corresponds to Implement Annotation config types, implement Annotation configuration and its validator along with its tests. Relates to #64 issue.
In a way of mimicking
openvariant/annotation/config_annotation.pyandopenvariant/annotation/annotation.pyfrom Python-based implementation.src/annotation/config.rs, which defines the Annotation structure.src/annotation/validator.rs, including the main functions for parsing and validating the format of Annotation YAML files.src/tests/test_annotation/, which can be run withmake test.For the moment, I didn't think about Warnings, only error. But they can be added.
If there are any tests missing, or if the validation is missing something, let me know. Also, I added tests inside
src/folder which I don't know if the best practice... because we have Python-based tests in the main folder astests/