PR: Feature: serialise CUDS model by Yaml script #322
Conversation
| # a class that is dynamically instantiated. A solution would be | ||
| # to define default values, e.g. 'None', for all parameters that | ||
| # do not have it defined yet. | ||
| comp_inst = comp_class(None, None) |
There was a problem hiding this comment.
I think we have to agree on having no manatory arguements for instantiating meta classes. Nothing you can do here.
There was a problem hiding this comment.
Yes, that would simplify CUDS to yaml code significantly. I'll try to make it work WITH the mandatory agruments right now, and we can remove that part of code when all default values are defined to CUDS components.
There was a problem hiding this comment.
Since the meta data specifies certain fields to be "required", it makes sense to have mandatory arguments, otherwise the "required" attribute is only a description in the yaml file that never gets executed or presented in any way in the class. Alternatively, it should be quite simple to add a property called "required_keys" to the generated class.
Related to how comp_class should be instantiated: The arguments in the generated __init__ may not be the same each time the class is generated, as the yaml file is loaded into a dictionary whose order is lost. Therefore I would suggest always instantiating a class with keyword arguments. i.e. Box(name='box1') instead of Box('box1') because one day the first argument might be something else that is not name
There was a problem hiding this comment.
@kitchoi, I don't think it's possible to instantiate classes dynamically here in that way. Doing it like comp_inst = comp_class(required_parameters[0] = value1) (if required_parameters[0] refers to CUBA.NAME, for instance) does not work. When I'm trying to instantiate the class that way, I get a SyntaxError: keyword can't be an expression.
There was a problem hiding this comment.
Could you try this?
init_kwargs = {required_parameters[0]: value1}
comp_inst = comp_class(**init_kwargs)
(and if required_parameters[0] is "CUBA.NAME", you probably want to convert it to "name")
There was a problem hiding this comment.
Thanks @kitchoi. This is what I was looking for. I'll rewrite the code to instantiate classes that way, so we can get rid of randomly feeding None's to the objects.
| for item in CC.iter(CUDSComponent): | ||
| self.assertEqual(item, None) | ||
|
|
||
| def test_save_CUDS_full(self): |
There was a problem hiding this comment.
This test fails for me. I was not able to fix it in my first try.
| C = CUDS(name='full', description='fully randomized model') | ||
|
|
||
| for key in KEYWORDS.keys(): | ||
| if key not in ['VERSION']: |
There was a problem hiding this comment.
Why do we exclude VERSION?
There was a problem hiding this comment.
Version() needs four mandatory arguments and therefore we can't use comp_inst = comp_class(None,None) (see serialisation.py L242) to instantiate it. I could try to modify the L242 so that it would check there how many supported parameters the class has and instantiate it with corresponding number of Nones.
| CCitem = CC.get(Citem.name) | ||
| self.assertEqual(CCitem.name, Citem.name) | ||
| for key in Citem.data: | ||
| self.assertEqual(Citem.data[key], CCitem.data[key]) |
There was a problem hiding this comment.
While trying to fix the failed test I got weired errors this line. Like numpy array errors.
| return random.choice([True, False]) | ||
|
|
||
| def _create_random_comp(self, key): | ||
| api = import_module('simphony.cuds.meta.api') |
There was a problem hiding this comment.
Can't we simply import this on top of the module?
There was a problem hiding this comment.
Yes, you're right. I'll fix that.
Current coverage is 68.58% (diff: 73.79%)@@ master #322 diff @@
==========================================
Files 118 119 +1
Lines 7390 7535 +145
Methods 0 0
Messages 0 0
Branches 875 918 +43
==========================================
+ Hits 5019 5168 +149
+ Misses 2333 2311 -22
- Partials 38 56 +18
|
|
Looks good to me. Please get rid of WIP tag whenever you are done. |
|
Thanks @mehdisadeghi . CUDS.data serialisation is still missing and I'm working on it right now. I'm writing data serialisation based on the modified data attribute which would be consistent with other objects having it defined. We can merge this feature after the data attribute is fixed. |
|
In fact, it's still an open question what should be stored in the CUDS.data DataContainer. Do we allow complicated CUDSComponents like Cfd() to be stored there, and what would be the use-case. Precisely, what's the distinction between a Cfd() object stored in CUDS.data and in CUDS._store? I think we can merge this branch now and deal with the data attribute later, perhaps together with the incompletely tested saving of different CUDSComponents. I'm removing the WIP tag, so please, @mehdisadeghi go ahead and merge if you're happy with this branch. |
tuopuu
changed the title
This PR adds a feature to save and load CUDS models from Yaml files. This is a work-in-progress version, but the users are encouraged to try it and test how it fits to different use-cases.
functions: