georgymh / decentralized-ml

Interoperable and decentralized machine learning.
Apache License 2.0
9 stars 5 forks source link

Config manager #7

Closed neeleshdodda44 closed 6 years ago

neeleshdodda44 commented 6 years ago

Sorry about the delay. Should be a straight forward pull request. Couple things (quoting the Trello card):

These "question entries" can be stored as a JSON in core/configuration/questions.json

Honestly I thought this was a textbook description of a CSV since every "row" has a question, , config section, key and default answer. So I just made those the columns and created a CSV. I still think it's easy to update, since adding to the question bank is simply adding a row.

Check setup mode is entered when the configuration file doesn't exist

Yes, I know configuration.ini exists right now. But the tests remove the file (using reset) at the beginning, so a new config is actually created for each test. Why don't I just use a new directory for testing? I could mock out ConfigurationManager in the components that depend on it, but I ultimately decided that it was important to test that each file can access the config in their respective ways. Because each test removes configuration.ini at the start and leave a copy of the default configuration.ini at the end, the functionality is still proven to be correct.

neeleshdodda44 commented 6 years ago

Summarizing what I said in meeting for clarity

I revamped the design of the class. __init__ takes optional parameters for config_filepath and input_function, which are attributes of the instance. bootstrap and get_instance take no parameters. So from a consumer's point of view (as long as the file path does not change), all that needs to be done is initialize the class and call get_instance to retrieve the instance.

The tests are a bit more complex. So in order to uphold all the design suggestions you made (singleton, keep bootstrap in __init__, don't use test variables, etc.) I decided to use fixtures. Each test file creates/alters a ConfigurationManager instance so that it knows where to pull/create the config file. The only exception is DMLScheduler, where all of that happens outside of the test methods and fixtures because the runners never finish running otherwise. Not sure if this worth worrying about for now, but I can create a TODO card to try to debug why fixtures don't work for the scheduler at a later point.

Tl;dr tests pass and from a consumer standpoint, I believe I've addressed all your concerns.

neeleshdodda44 commented 6 years ago

Looks like we've come into agreement with the final design of ConfigurationManager. Merging now.