Overhaul configuration file parsing #246

desilinguist · 2015-07-14T20:00:32Z

Use a new custom parser class SKLLConfigParser based on ConfigParser.
Move all of the configuration parsing code to config.py and out of experiments.py.
Add a validate() method to the config parser that raises a KeyError if:
- unrecognized options are specified
- options are specified in more than one section
- options are specified in the wrong section (need to improve the way to check the validity of configuration file #223)
Add unit tests for the above validation checks
Update tests to deal with the above changes
Update requirements_rtd.txt to install the latest version of ConfigParser (v3.5.0b2)

- Use a new custom parser class `SKLLConfigParser` based on `ConfigParser`. - Move all of the configuration parsing code to `config.py` and out of `experiments.py`. - Add a `validate()` method to the config parser that raises a KeyError if: - unrecognized options are specified - options are specified in more than one section - options are specified in the wrong section - Add unit tests for the above validation checks - Update test files to deal with the above changes - Update `requirements_rtd.txt` to install the latest version of `ConfigParser` (v3.5.0b2)

…onfig-parsing Conflicts: skll/experiments.py tests/test_classification.py tests/utils.py

desilinguist · 2015-07-14T20:01:36Z

@dan-blanchard please do look at this one too since it's a somewhat major change.

aoifecahill · 2015-07-16T17:55:24Z

tests/test_input.py

+    train_dir = join(_my_dir, 'train')
+    output_dir = join(_my_dir, 'output')
+
+    # make a simple config file that has an invalid option


minor: comment should be updated to reflect that the problem with this config is that there is a duplicate key (that comes from the fill_in_config_options method)

aoifecahill · 2015-07-16T21:55:27Z

This looks good to me, I didn't spot anything obvious that might cause problems.

dan-blanchard · 2015-07-17T13:19:07Z

I did a little PEP8 clean up here and there, but otherwise this looks great! 👍

desilinguist · 2015-07-17T13:21:59Z

Thanks, Dan! I will address Aoife's comment and merge soon as the build passes.

dan-blanchard · 2015-07-17T14:17:45Z

I fixed the later comments to reflect the names of the functions, but I guess I missed that one.

desilinguist · 2015-07-17T14:47:19Z

Sweet! Thanks for that!

Actually, I just realized I didn't address #213 in this PR. Do you think that's important for this new release?

dan-blanchard · 2015-07-17T14:51:47Z

It would be nice to have fixed for this release, since we know it's bitten some new users when trying to follow the tutorial.

desilinguist · 2015-07-17T14:59:55Z

Okay, I will add that to this PR then and try to add some tests for it too.

landscape-bot · 2015-07-17T15:58:32Z

Repository health increased by 0.13% when pulling 653fc44 on feature/overhaul-config-parsing into 2ad3b65 on master.

4 new problems were found (including 0 errors and 2 code smells).
3 problems were fixed (including 0 errors and 1 code smell).

landscape-bot · 2015-07-17T16:56:15Z

Repository health increased by 0.13% when pulling 653fc44 on feature/overhaul-config-parsing into 2ad3b65 on master.

4 new problems were found (including 0 errors and 2 code smells).
3 problems were fixed (including 0 errors and 1 code smell).

- Get the absolute path of the config file right at the start. - Look for and create all files/paths relative to the (absolute) config file path. - Add two new unit tests. - Fix an issue with how the default value of `class_map` was being handled due to the updated defaults.

desilinguist · 2015-07-17T21:50:55Z

@dan-blanchard and @aoifecahill, can you please take a look again? I just committed some changes to address #213.

landscape-bot · 2015-07-17T21:54:01Z

Repository health increased by 0.16% when pulling e508977 on feature/overhaul-config-parsing into 2ad3b65 on master.

4 new problems were found (including 0 errors and 2 code smells).
3 problems were fixed (including 0 errors and 1 code smell).

landscape-bot · 2015-07-17T22:25:54Z

Repository health increased by 0.16% when pulling 3a08303 on feature/overhaul-config-parsing into 2ad3b65 on master.

4 new problems were found (including 0 errors and 2 code smells).
3 problems were fixed (including 0 errors and 1 code smell).

dan-blanchard · 2015-07-18T00:06:51Z

skll/config.py

+
+    # get all the input paths and directories (without trailing slashes)
+    train_path = config.get("Input", "train_directory").rstrip('/')
+    test_path = config.get("Input", "test_directory").rstrip('/')


This won't work on Windows.

This has been in there since the beginning so I guess it has never worked?

- Replace `abspath()` with `realpath()` when getting the full path of the config file. - Strip out `os.sep` instead of slashes from the end of paths.

landscape-bot · 2015-07-18T04:38:06Z

Repository health increased by 0.16% when pulling aa2c341 on feature/overhaul-config-parsing into 2ad3b65 on master.

4 new problems were found (including 0 errors and 2 code smells).
3 problems were fixed (including 0 errors and 1 code smell).

desilinguist · 2015-07-18T12:50:28Z

I replaced the / with os.sep which should work on Windows too.

dan-blanchard · 2015-07-18T12:53:49Z

tests/test_input.py

+
+def test_config_parsing_relative_input_path():
+
+    train_dir = '../train'


This should also be:

train_dir = join('..', 'train')

Ah, good call.

desilinguist · 2015-07-18T14:39:10Z

So, is this okay to be merged once I address your last comment?

dan-blanchard · 2015-07-18T15:00:06Z

Definitely.
On Sat, Jul 18, 2015 at 10:39 AM Nitin Madnani notifications@github.com
wrote:

So, is this okay to be merged once I address your last comment?

—
Reply to this email directly or view it on GitHub
#246 (comment)
.

…ul-config-parsing Overhaul configuration file parsing

desilinguist added 2 commits July 14, 2015 11:07

Merge branch 'feature/several-minor-bugfixes' into feature/overhaul-c…

80f291c

…onfig-parsing Conflicts: skll/experiments.py tests/test_classification.py tests/utils.py

desilinguist assigned aoifecahill Jul 14, 2015

aoifecahill reviewed Jul 16, 2015
View reviewed changes

PEP8 overhauled config parser files

653fc44

Replace FileNotFoundError in unit test for python 2.7

3a08303

dan-blanchard reviewed Jul 18, 2015
View reviewed changes

Minor changes

aa2c341

- Replace `abspath()` with `realpath()` when getting the full path of the config file. - Strip out `os.sep` instead of slashes from the end of paths.

dan-blanchard reviewed Jul 18, 2015
View reviewed changes

Fix another windows-unfriendly statement.

3b58ceb

desilinguist added a commit that referenced this pull request Jul 18, 2015

Merge pull request #246 from EducationalTestingService/feature/overha…

1ecd54a

…ul-config-parsing Overhaul configuration file parsing

desilinguist merged commit 1ecd54a into master Jul 18, 2015

desilinguist deleted the feature/overhaul-config-parsing branch July 18, 2015 19:11

This was referenced Jul 18, 2015

need to improve the way to check the validity of configuration file #223

Closed

Config file paths are relative to current working directory instead of config file location #213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul configuration file parsing #246

Overhaul configuration file parsing #246

desilinguist commented Jul 14, 2015

desilinguist commented Jul 14, 2015

aoifecahill Jul 16, 2015

aoifecahill commented Jul 16, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

desilinguist commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

dan-blanchard Jul 18, 2015

desilinguist Jul 18, 2015

landscape-bot commented Jul 18, 2015

desilinguist commented Jul 18, 2015

dan-blanchard Jul 18, 2015

desilinguist Jul 18, 2015

desilinguist commented Jul 18, 2015

dan-blanchard commented Jul 18, 2015


		def test_config_parsing_relative_input_path():

		train_dir = '../train'

Overhaul configuration file parsing #246

Overhaul configuration file parsing #246

Conversation

desilinguist commented Jul 14, 2015

desilinguist commented Jul 14, 2015

aoifecahill Jul 16, 2015

Choose a reason for hiding this comment

aoifecahill commented Jul 16, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

dan-blanchard commented Jul 17, 2015

desilinguist commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

desilinguist commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

landscape-bot commented Jul 17, 2015

dan-blanchard Jul 18, 2015

Choose a reason for hiding this comment

desilinguist Jul 18, 2015

Choose a reason for hiding this comment

landscape-bot commented Jul 18, 2015

desilinguist commented Jul 18, 2015

dan-blanchard Jul 18, 2015

Choose a reason for hiding this comment

desilinguist Jul 18, 2015

Choose a reason for hiding this comment

desilinguist commented Jul 18, 2015

dan-blanchard commented Jul 18, 2015