search

cnrl / CoNeX

Cortical Network for everything!
MIT License
3 stars 0 forks source link

feat: Add config loader and store method #23

Closed realamirhe closed 1 year ago

realamirhe commented 1 year ago

Make function call is still needed after the configuration file is saved and loaded. but it seems modular to me. open for suggestions

Related to https://github.com/cnrl/CoNeX/issues/17

realamirhe commented 1 year ago

The json support can easily be added, but for calling make at the end I'm not sure it's the best way to handle it, I was thinking about the more generic loader, which load and make each module with more ease, here the user must copy and paste some code blocks for simple configuration restoration.

But I'm okay to call make at the end of each load section :)

saeedark commented 1 year ago

@atenagm1375 Load should not call the make method. make method constructs the dictionary a Module uses. However, it's reasonable to have a function that makes a config instance automatically. Since we have multiple classes for config I'm lost on how an automake for config should work. (the body of the make method for each class is different...)

atenagm1375 commented 1 year ago

@atenagm1375 Load should not call the make method. make method constructs the dictionary a Module uses. However, it's reasonable to have a function that makes a config instance automatically. Since we have multiple classes for config I'm lost on how an automake for config should work. (the body of the make method for each class is different...)

I still don't get the point. Upon calling load, the user should be able to retrieve either the config instance or the dictionary itself. Right now, nothing is returned by load and the user should instantiate a Config, call load on it, and then call a make. All this is redundant and inconsistent. I know that make is not the same for all config types, but load is a method inherited in all of them and can be overridden. Tell me which part is exactly impractical, so that we can come up with a solution.

saeedark commented 1 year ago

First sorry for the unrelated commit.

load_from_yaml now returns a dictionary of instances. @atenagm1375 @realamirhe

saeedark commented 1 year ago

Why haven't we merged this already?

atenagm1375 commented 1 year ago

Why haven't we merged this already?

waitning for @realamirhe approval

realamirhe commented 1 year ago

JSON structure must also be added, I'm going to add it right now

saeedark commented 1 year ago

I think The easiest way is to find a custom jsonEncoder or write one that supports callables.

realamirhe commented 1 year ago

I think The easiest way is to find a custom jsonEncoder or write one that supports callable.

Do you have any idea how we could have the encoding for all JSON structures inside the default_loader? @saeedark

saeedark commented 1 year ago

I think The easiest way is to find a custom jsonEncoder or write one that supports callable.

Do you have any idea how we could have the encoding for all JSON structures inside the default_loader? @saeedark

honestly no.

realamirhe commented 1 year ago

@atenagm1375 if there is a task with higher priority, it might be better to merge this PR and wait for a more mature json load-save with jsonEncoder on the mapping-proxy structures. If not I'm going to test some more implementations of default-loader till next session