Components refers to models of a specific lib#75
Conversation
…r and return type adjustments
…update test cases for library structure
…nce dependency handling; add new test cases for model redefinition scenarios
…e for clearer exception message
…ated tests and YAML files for consistency
sylvlecl
left a comment
sylvlecl
left a comment
There was a problem hiding this comment.
Looks good overall:
one typo + minor improvement suggestions which are not "must have"
| def resolve_library( | ||
| input_libs: List[InputLibrary], preloaded_libs: Optional[List[Library]] = None | ||
| ) -> Library: | ||
| ) -> Dict[str, Library]: |
There was a problem hiding this comment.
would be even better to create a new container class for the libraries, like:
class LibrariesRepository:
...
def find_model(lib_id: str, model_name: str):
...
def find_fully_qualified_model(fq_model_name: str):
... # also checks validity of the FQ name and split itwhich would internally have the dictionary
There was a problem hiding this comment.
Agree, will do it in another PR
|
|
||
| return output_lib | ||
| if first_dependency in import_stack: | ||
| raise Exception("Circular import in yaml libraries") |
There was a problem hiding this comment.
In practice circular imports are not a blocking issue because models only depend on ports, and ports don't depend on other ports.
So it would work to first create ports of all libraries in a first step, and then resolve all models of all libraries in a second step.
But ok to leave it this way
There was a problem hiding this comment.
I agree, however I find it cleaner to avoid circular import (maybe in the future we could define model extensions, or having other feature for which circular imports are blocking). And also I do not want to spend too much time now changing the logic of imports, will do it only if it is needed to replicate the C++ behavior.
| def _resolve_component( | ||
| libraries: dict[str, Library], component: InputComponent | ||
| ) -> Component: | ||
| lib_id, model_id = component.model.split(".") |
There was a problem hiding this comment.
Should we accept multiple levels of lib, and only split on the last dot ?
like rte.drd.model-of-the-future ?
If not we should check that there is indeed only one dot.
Optionally this could be delegated to a container of libraries, see proposition above.
There was a problem hiding this comment.
Like the previous comment, I let the reflexions on this question for the industrial part, the Python code will just follow C++ choices
3 participants
Update the libraries and system yaml files to be fully compatible with the one defined in C++:
lib_id.modelmodel-librairiesis allowed, although it is not used now (nor is it in the C++ version for now)The Python format is still ahead of the C++ format (as of 2 April 2025) as it allows to handle dependencies between librairies to import port type declarations