Load Obos first and fallback#474
Conversation
This reverts commit 03ddef5.
The crate changed how files are stored. Instead of splitting ontology id from version with a "_" it uses an "@" now
psnairne
left a comment
psnairne
left a comment
There was a problem hiding this comment.
Only one important comment which is the question about the repeated logic
| if let Some(onto) = self.get_cached_ontology(ontology_ref) { | ||
| return Ok(onto.ontology.clone()); | ||
| } | ||
|
|
||
| for r in self.registry.list()? { | ||
| if r.version().to_string() == ontology_ref.version() | ||
| && r.ontology_id().to_lowercase() == ontology_ref.prefix_id().to_lowercase() | ||
| { | ||
| return match r.file_type() { | ||
| FileType::Json => self.build_ontolius_ontology(ontology_ref), | ||
| FileType::Obo => self.build_obodoc_ontology(ontology_ref), | ||
| FileType::Owl => Err(FactoryError::CantBuild { | ||
| reason: format!( | ||
| "OWL files are not supported. Got a configuration for {}", | ||
| r | ||
| ), | ||
| }), | ||
| }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct me if I'm wrong, but this seems to be doing the same thing twice? Was there a problem with the logic in the get_cached_ontology function?
There was a problem hiding this comment.
Its not doing the same thing twice. get_cached_ontology looks into memory, while this logic looks on the hard drive.
There was a problem hiding this comment.
Ah I see. So I suppose adding this logic allows for the hypothetical situation where
ontology_metadata.json_file_location.is_some()
|| ontology_metadata.obo_file_location.is_some()
becomes False, but we still have the file on our system. I suppose it's more robust that's true!
There was a problem hiding this comment.
Actually, its here to not download unloadable obos over and over again.
| ontology_ref.as_version(), | ||
| FileType::Json, | ||
| ); | ||
| self.registry.unregister(reg_key)?; |
There was a problem hiding this comment.
So, that you do not keep unloadable ontologies in your registry.
| if ontology_metadata.json_file_location.is_some() | ||
| || ontology_metadata.obo_file_location.is_some() | ||
| { | ||
| if let Ok(ontology) = self.build_obodoc_ontology(ontology_ref) { |
There was a problem hiding this comment.
The only issue with this logic is that if obodoc build is failing for some other reason, e.g. an error with our build Ontology from obodoc code, we won't know, and will just get a JSON instead.
I think I slightly prefer the previous logic - if there is an obo file in the metadata then we should be able to build the ontology from the obo file. But also I don't mind too much
There was a problem hiding this comment.
The only issue with this logic is that if obodoc build is failing for some other reason, e.g. an error with our build Ontology from obodoc code, we won't know, and will just get a JSON instead.
But that was the whole point of this PR.
If we can not build an obo ontology, we try to build it from a json. Why is that a problem?
I think I slightly prefer the previous logic - if there is an obo file in the metadata then we should be able to build the ontology from the obo file. But also I don't mind too much
And why do you prefer that logic?
It makes less ontologies accessible and fully relies on FastObos ability to build an ontology, if Json and obo are available.
There was a problem hiding this comment.
Advantage of your way = more robust
Advantage of previous way = errors with OBO or JSON parsing are made clear.
Anyway, I think it's fine both ways!
2 participants
No description provided.