Skip to content
This repository was archived by the owner on Aug 27, 2024. It is now read-only.

Remove YAML Converter #12

Merged
merged 2 commits into from
Jul 21, 2024
Merged

Remove YAML Converter #12

merged 2 commits into from
Jul 21, 2024

Conversation

DanInci
Copy link
Contributor

@DanInci DanInci commented Jul 19, 2024

Remove YAML converter and leave only LinkML converter in the codebase

@DanInci DanInci changed the base branch from main to benchmark_model_updates July 19, 2024 08:48
@almutlue
Copy link
Member

Suggestion:
I think ideally the converter class would be removed as a whole and the Benchmark class has a model attribute that provides the omnischema.model.Benchmark. This would be one layer of nesting less and more readable. I know this requires much more refactoring, but I think it will pay off in future. e.g.:

class Benchmark:
    def __init__(self, 
                 bench_yaml: Path, 
                 out_dir: str ='out'):
        
        self.model = dict_from_yaml(bench_yaml)
        self.out_dir = out_dir

@imallona
Copy link
Member

No strong opinion about ☝️ but related. I'm sort of conflicted with the YAML parsing, I still think we'd be better off with a LinkML-free YAML parsing. The dependency, and https://github.com/omnibenchmark/omni-schema, seem too large to me to parse an uncomplicated YAML. Perhaps we could make things slimmer in future iterations, both abstraction-wise and tool-wise? Happy to discuss anytime

@DanInci
Copy link
Contributor Author

DanInci commented Jul 21, 2024

No strong opinion about ☝️ but related. I'm sort of conflicted with the YAML parsing, I still think we'd be better off with a LinkML-free YAML parsing. The dependency, and https://github.com/omnibenchmark/omni-schema, seem too large to me to parse an uncomplicated YAML. Perhaps we could make things slimmer in future iterations, both abstraction-wise and tool-wise? Happy to discuss anytime

I agree with this, for now LinkML provides a lot of out-of-the-box validations and parsing for us. Maybe when we'll have a stable schema, we could implement our own parser. But currently LinkML saves a lot of development effort.

@DanInci
Copy link
Contributor Author

DanInci commented Jul 21, 2024

Suggestion: I think ideally the converter class would be removed as a whole and the Benchmark class has a model attribute that provides the omnischema.model.Benchmark. This would be one layer of nesting less and more readable. I know this requires much more refactoring, but I think it will pay off in future. e.g.:

class Benchmark:
    def __init__(self, 
                 bench_yaml: Path, 
                 out_dir: str ='out'):
        
        self.model = dict_from_yaml(bench_yaml)
        self.out_dir = out_dir

Changed the model.Benchmark interface to be the same as the one you mentioned, although I kept the LinkMLConverter class, since it contains some logic that I find different than the one I expect in the Benchmark interface, e.g. computing the explicit vs implicit class, the order of stages and so on.

Despite it introducing an additional layer of nestedness, I believe it will be useful in the future, in case we decide to swap the parser or model schema. This will maintain a decoupling between low-level parsing logic and higher-level benchmark model logic.

@DanInci DanInci merged commit 7ed2267 into benchmark_model_updates Jul 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants