Decide on preferred or recommended config file format, to enable an initial ConfigurationManager type class somewhere #21

Closed thompson318 closed 4 years ago

In GitLab by @MattClarkson on Nov 16, 2018, 09:50

Any thoughts on whats preferred?

conf.py - good for live code, but Im thinking configuration shouldnt need live code. Risky for medical apps.
yaml/ini/json - I (Matt) has no real preference. ini is easy to edit, and probably sufficient.

Advice needed.

@StephenThompson @ThomasDowrick

In GitLab by @ThomasDowrick on Nov 16, 2018, 10:30

Any settings read in from a file should probably go into a Python dictionary. If we take this as a starting point, then yaml or json makes the most sense, as they can both be imported straight into a dictionary.

I'd personally go for yaml, as it is easier to write a valid yaml file than a json one.

In GitLab by @ThomasDowrick on Nov 16, 2018, 17:50

Moving discussion here about which package to put config manager in.

Looking at our previous decision on namespaces it would fall under scikit-surgerycore (which doesn't exist yet) as there won't be any dependencies on external libraries.

Also, under the same logic, scikit-surgeryalgorithms and scikit-surgerytransforms should be made part of the core library (e.g. core.algorithms and core.transforms) as they only have numpy dependencies. I also prefer this as it keeps the number of different packages to a minimum.

In GitLab by @StephenThompson on Nov 20, 2018, 14:05

yaml or just plain python for me. yaml should be more portable, but it does mean an extra dependency and something else to learn. We want to be careful that we keep things as simple as possible.

In GitLab by @MattClarkson on Nov 20, 2018, 15:48

I can create a new project for scikit-surgerycore, and move this ticket?

Also - are we happy that algorithms and transforms are moved? It does reduce the number of libraries.

In GitLab by @StephenThompson on Nov 20, 2018, 15:58

I'm not convinced about scikit-surgerycore. If we try and do too much with it it will have too many dependencies that will then break whatever depends on it. Alternatively we let it do very little to minimise dependencies, and then it's not very useful. I'd rather have lot's of libraries for the minute.

In GitLab by @MattClarkson on Nov 20, 2018, 16:02

ok, final question before I run for train. Is there still a place for something like the ConfigurationManager we mentioned? to scikit-surgerycore could be a central thing to assist with gluing other things together?

In GitLab by @ThomasDowrick on Nov 20, 2018, 16:17

@StephenThompson - when you say dependenices, do you mean other Python pacakges? scikit-surgerycore, in our orignal discussion on it, would only have numpy as a dependency, so this shouldn't be an issue.

I would veer towards keeping the number of packages down. If we have too many, then I imagine people will try and just use the scikit-surgery master package that installs everything, rather than figuring out which ones they want, which defeats the point of not putting everything in a single package in the first place.

In GitLab by @StephenThompson on Nov 20, 2018, 16:20

From the point of view of an individual library (eg. scikit-surgerynditracker) running things through the configuration manager adds a layer of complexity and abstraction that may not be helpful. However I think when it comes time to assemble an application a configuration manager would be very useful.

In GitLab by @StephenThompson on Nov 20, 2018, 16:25

@ThomasDowrick At the moment algorithms and transforms only depend on numpy and six, so I guess they could go into core OK. A configuration manager would presumably require pyyaml so should not be part of core.

In GitLab by @ThomasDowrick on Nov 20, 2018, 16:29

Good point about pyyaml - I had assumed yaml support comes built in to Python but upon closer inspection it doesn't.

In GitLab by @MattClarkson on Nov 20, 2018, 19:26

Agree with @StephenThompson with regards to wanting individual components like a tracker and whatever to be testable independently. This means independently of our own modules, so encourage good unit testing and good coverage. Each module should have minimum dependencies in general, just to reduce dependency hell.

But if we made a new package that was just for config, or at least, the first bit of functionality was for configuration management would it be

scikit-surgeryconfig (if so, I can't see many classes in here).
scikit-surgeryyaml (i.e. for all things yamelly, but then its not an obvious name).

In GitLab by @ThomasDowrick on Nov 21, 2018, 08:25

Now that I know YAML isn't built into Python - I would no longer recommend using YAML for the config and probably go for JSON instead, and just have the config bit as part of scikit-surgerycore.

In GitLab by @StephenThompson on Nov 21, 2018, 09:07

I'm not clear on what we gain with json over config files written in python i.e.

some JSON:

x = '{ "name":"John", "age":30, "city":"New York"}'

versus

a Python object (dict):

x = { "name": "John", "age": 30, "city": "New York" } The syntax is nearly identical, but the user has to learn a new language. What is it we want from our config manager?

In GitLab by @StephenThompson on Nov 21, 2018, 09:09

Sorry about the preceding post's formatting.

In GitLab by @MattClarkson on Nov 21, 2018, 09:32

i just want to load settings from an easily editable text file.

In GitLab by @ThomasDowrick on Nov 21, 2018, 11:02

The difference between JSON and a config.py file is fairly trivial for our case I suppose, in terms of how the data is represented. In terms of practical differences I would say it is more 'Pythonic' to use JSON:

It is more apparent that a JSON file will contain dictionary data, rather than just having it in a config.py file, which some programs might be using for other code.

The JSON could be parsed by non-python programs (although not sure if this is a relevant use case for us).

The method of loading the data differs:

from config import x

vs

x = json.loads('config.json')

In the first instance, it isn't clear that a dictionary is being imported, and the location of the config file needs to be known in advance.

In GitLab by @StephenThompson on Nov 21, 2018, 11:55

I tend to agree that using json would make it clearer what's config and what's code. So I suggest: Algorithms, transforms, and a new config manager go into scikit-core and we must continue to make sure that scikit-core only requires numpy (and six?)

In GitLab by @ThomasDowrick on Nov 21, 2018, 12:13

Agreed

In GitLab by @MattClarkson on Nov 21, 2018, 13:18

Jolly good, we are converging. I think one of the original intentions of the naming convention scikit-blah was that each scikit package generally is assumed to use scipy default packages like numpy, scipy, matplot lib and pandas. I'd rule out matplot lib for scikit-surgerycore as visualisation and plotting should be separate, but we might need to allow pandas or scipy at some point, if it is deemed necessary perhaps.... but lets not add it until we need it.

In GitLab by @MattClarkson on Nov 21, 2018, 20:51

ok, i started scikit-surgerycore, merged algorithms code and transform code in, and will move this issue to that project to continue with ConfigurationManager type thing, based on json.

In GitLab by @MattClarkson on Nov 21, 2018, 21:02

assigned to @MattClarkson and unassigned @StephenThompson

SciKit-Surgery / scikit-surgery