Config refactor #298

antalszava · 2020-02-21T20:46:22Z

Context:
Currently, to configure a connection to a remote service the Configuration class is used which encompasses the creation and updating of configuration-related data, allowing environment variables and configuration files to be used. However, no keyword arguments can be passed to this class for configuration. Furthermore, storing an instance of this class that it is available from different components of SF (e.g. RemoteEngine, LocalEngine) seems to be problematic, as a global Configuration instance would be needed for such use cases.

Description of the Change:
This is the first part of replacing the Configuration class with functions, adding the load_config and auxiliary functions.

The configuration object (that is a nested dictionary) would be created based on the following (order defines the importance, going from most important to least important):

keyword arguments passed to load_config
data contained in environmental variables (if any)
data contained in a configuration file (if exists)

If a user wants to simply use a configuration, the configuration object is available from confiugration.py which calls on load_config without any arguments. This solves the issue of having a global configuration if needed. A default configuration object called DEFAULT_CONFIG is also available containing pre-defined data for configuration.

Further changes regarding current implementation

If there are several configuration files at multiple locations, the very first to be found will be regarded
Moved the creation of a default config object into a function: now the defaults are through having the kwargs.get("option", default) calls
Iteration through the dictionary will occur multiple times -> means two iterations but creates a cleaner logic

Further comments

Functionality and tests have been removed for saving configuration files as well, these will be added in the subsequent PR
Tests that were failing due to parts of SF that will be soon refactored were marked xfail

Benefits:

The load_config function can be called anytime for configuration, without having to keep and update the state that was contained in a Configuration instance.
There is now an opportunity to load the configuration from keyword arguments and environment variables as well.
When parsing environment variables, a logic for parsing integers has been added.
The current use cases were taken into consideration.
More tests were added.

Possible Drawbacks:
Currently, all three options of keyword arguments, environment variables and configuration file are taken into consideration, eventually giving a union of the options while also taking into consideration the precedence. For now, this wouldn't seem to be a problem with limited details available.

Related GitHub Issues:
N/A

…variable test; adding teardown logic for env vars; correcting typo

codecov · 2020-02-21T21:57:16Z

Codecov Report

Merging #298 into demo will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             demo     #298   +/-   ##
=======================================
  Coverage   96.86%   96.86%           
=======================================
  Files          48       48           
  Lines        6222     6222           
=======================================
  Hits         6027     6027           
  Misses        195      195

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0063663...0063663. Read the comment docs.

… DEFAULT_CONFIG as module attribute

…r kwargs update; add logic for integer parsing from env vars; add tests

co9olguy

Looks great @antalszava! Especially the thorough tests and clear separation of labour of the function.

Left a few suggestions for polishing etc.

tests/frontend/test_api_client.py

tests/frontend/test_engine.py

tests/frontend/test_configuration.py

strawberryfields/configuration.py

Co-Authored-By: Nathan Killoran <[email protected]>

…rryfields into config_refactor

antalszava · 2020-02-25T02:53:52Z

Locally I got the following for configuration.py:

Coverage for strawberryfields/configuration.py : 100%
69 statements   69 run 0 missing 3 excluded

Whereas the changes in other files will be affected by an upcoming refactor there.

josh146

This is great, thanks @antalszava! While it looks like a lot of comments, most are just polish --- the new config logic is 💯 times clearer and better than the previous iteration!

strawberryfields/configuration.py

josh146 · 2020-02-25T11:25:48Z

strawberryfields/configuration.py

+ hostname (str): the name of the host to connect to
+ use_ssl (bool): specifies if requests should be sent using SSL
+ port (int): the port to be used when connecting to the remote service
+ debug (bool): determines if the debugging mode is requested


I'm thinking it might be best to move all the configuration options to the module docstring. This has several advantages:

Less redundancy --- configuration options only need to be specified in one place, and linked from everywhere else

We can use Sphinx glossary notation to add more details

It is user facing, whereas these docstrings aren't

I've added them to the module docstring :) Added the list of environment variables, let me know if you think they need more adjustments :)

josh146 · 2020-02-25T11:29:23Z

strawberryfields/configuration.py

+ for section, sectionconfig in config.items():
+ for key in sectionconfig:
+ if key in other_config[section]:
+ config[section][key] = other_config[section][key]


Could this be made more succinct using the dict.update method?

I've changed it so, however, will have to add certain validity check most probably, such that no problems can arise from updating with invalid configurations.

strawberryfields/configuration.py

josh146 · 2020-02-25T11:41:48Z

strawberryfields/configuration.py


+ update_from_environment_variables(config)
+
+ config_from_keyword_arguments = {"api": kwargs}


It might make sense to define the kwargs as 'belonging' to the "api" section when the defaults/types are defined.

Would that be referring to the defaults used in create_config?

One of the problems I came across was that if I use create_config and kwargs here, then having defaults will result in an unexpected overwrite.

That is

In [1]: import strawberryfields.configuration as conf In [3]: config = { ...: "api": { ...: "authentication_token": 'NotDefaultAuth', ...: "hostname": 'NotDefaultHostname', ...: "use_ssl": True, ...: "port": 123456789, ...: "debug": False ...: } ...: } ...: other_config = conf.create_config(authentication_token="MyToken") ...: conf.update_config(config, other_config) ...: config Out[3]: {'api': {'authentication_token': 'MyToken', 'hostname': 'localhost', 'use_ssl': True, 'port': 443, 'debug': False}}

where 'hostname' and 'port' were overwritten, whereas they were not defined in kwargs.

Ah, I see. This is because the other_config is replacing the default values 🤔

strawberryfields/configuration.py

tests/frontend/test_configuration.py

josh146 · 2020-02-25T11:49:32Z

tests/frontend/test_configuration.py

+ assert v != parsed_value
+
+ # Tear-down
+ del os.environ[env_var]


Mentioned this earlier, but better to use monkeypatch.setenv() here to avoid side effects 🙂

… module level attributes

…dingly; change load_config using it

co9olguy

You've addressed all my concerns @antalszava

I notice a lot of small changes (e.g., to function names, signatures, and return types). I assume these come out of other reviews, so I will defer to e.g., @josh146 for final approval

strawberryfields/configuration.py

…tring, inserting refs to these in function docstrings, correcting Kwargs section naming in docstrings

antalszava · 2020-02-26T01:13:35Z

Reason for the force-push: Traversing the parents of each commit starting from the source 43d385d, it can be seen that the master branch was merged accidentally.

josh146

💯 Looks good from my end!

strawberryfields/configuration.py

josh146 · 2020-02-26T01:51:48Z

strawberryfields/configuration.py

+ * **hostname (str)** (*optional*): the name of the host to connect to
+ * **use_ssl (bool)** (*optional*): specifies if requests should be sent using SSL
+ * **port (int)** (*optional*): the port to be used when connecting to the remote service
+ * **debug (bool)** (*optional*): determines if the debugging mode is requested


The bullet point list needs to be de-indented to match the indentation level of The following... above.

josh146 · 2020-02-26T01:52:28Z

strawberryfields/configuration.py

+ * SF_API_HOSTNAME
+ * SF_API_USE_SSL
+ * SF_API_DEBUG
+ * SF_API_PORT


This is sufficient for now, but we should create a new ticket to fix up the configuration keys documentation 🙂

strawberryfields/configuration.py

josh146 · 2020-02-26T01:54:15Z

strawberryfields/configuration.py


+ update_from_environment_variables(config)
+
+ config_from_keyword_arguments = {"api": kwargs}


Ah, I see. This is because the other_config is replacing the default values 🤔

strawberryfields/configuration.py

tests/frontend/test_configuration.py

…d tests

Co-Authored-By: Josh Izaac <[email protected]>

… with kwargs, adding port to file and env vars

antalszava · 2020-02-26T03:40:08Z

These last bit of modifications included:

removing the update_config function
adding the keep_valid_options function and adding tests.

The keep_valid_options function makes sure that only options that are recognized can be included coming from a configuration file or keyword arguments.

These modifications make up for using the dict.update function where applicable. As currently the only sectionconfig is "api", this is used more than previously. This can be extended to more complex code once new sectionconfigs are introduced (and potentially reintroducing the logic from update_config)

Further changes were carried out to configuration.rst to update it with:

keyword arguments
environment variables
port (where applicable)

josh146

All comments are minor, this is in very good state :)

strawberryfields/configuration.py

josh146 · 2020-02-26T03:47:22Z

doc/introduction/configuration.rst

+* SF_API_AUTHENTICATION_TOKEN
+* SF_API_HOSTNAME
+* SF_API_USE_SSL
+* SF_API_PORT


I would combine the two sections above into one, something like (feel free to modify/clean):

Configuration options ********************* **authentication_token (str)** (*required*) The authentication token to use when connecting to the API. Will be sent with every request in the header. Corresponding environment variable: ``SF_API_AUTHENTICATION_TOKEN`` **hostname (str)** (*optional*) The hostname of the server to connect to. Defaults to ``localhost``. Must be one of the allowed hosts. Corresponding environment variable: ``SF_API_HOSTNAME`` **use_ssl (bool)** (*optional*) Whether to use SSL or not when connecting to the API. True or False. Corresponding environment variable: ``SF_API_USE_SSL`` **port (int)** (*optional*) The port to be used when connecting to the remote service. Corresponding environment variable: ``SF_API_PORT``

Oh thanks for that!

josh146 · 2020-02-26T03:49:12Z

strawberryfields/configuration.py

+.. warning::
+
+ The following configuration options are available:
+ :doc:`/introduction/configuration`.


For consistency with our other developer pages:

.. warning:: Unless you are a Strawberry Fields developer, you likely do not need to access this module directly. See more details regarding Strawberry Fields configuration and available configuration options on the :doc:`/introduction/configuration` page.

Sure! Here the first part will already be included from https:/XanaduAI/strawberryfields/blob/demo/doc/code/sf_configuration.rst

so I'll add the reworded second part :)

josh146 · 2020-02-26T03:49:46Z

strawberryfields/configuration.py

 }

-BOOLEAN_KEYS = ("debug", "use_ssl")
+BOOLEAN_KEYS = {"use_ssl"}
+INTEGER_KEYS = {"port"}


Now that the above DEFAULT_CONFIG_SPEC specifies the types, are these still needed?

Oh not at all, thanks for that!

strawberryfields/configuration.py

josh146 · 2020-02-26T03:51:14Z

strawberryfields/configuration.py

+VALID_KEYS = set(create_config()["api"].keys())
+DEFAULT_CONFIG = create_config()
+configuration = load_config()
+config_filepath = get_config_filepath()


This is very nice :)

Co-Authored-By: Josh Izaac <[email protected]>

…t module attributes, reword module docstring

…rryfields into config_refactor

antalszava added 6 commits February 18, 2020 13:55

First go with having function in the configuration.py

7e67e68

First draft

0efccb6

Logic of load_config before testing

24952e0

Testing config object creation, look_for_config_file function

7d62a23

Unit tests for the parts making up load_config

e44a1b7

Unit test for load_config; removing MagicMock from parse_environment_…

e0afae1

…variable test; adding teardown logic for env vars; correcting typo

antalszava added 10 commits February 21, 2020 17:07

Marking xfail API Client tests (being refactored in another PR)

3ca6a5b

Marking xfail API Client tests (being refactored in another PR)

abdc0af

Adding xfail mark on another API Client test

2995cd9

Docstrings, reorganizing parse_environment_variables function, adding…

0770172

… DEFAULT_CONFIG as module attribute

Add create config object test with every keyword argument

e7eb96a

Adding load_config tests using each units separately; change logic fo…

7bc48ca

…r kwargs update; add logic for integer parsing from env vars; add tests

Test docstrings

fc5619a

Default config refactor

9109a46

Add absolute path test

653f566

Saving path, linting, isort

984c5a5

antalszava marked this pull request as ready for review February 24, 2020 21:05

antalszava requested review from josh146, pauljxtan and co9olguy February 24, 2020 21:08

Modify dict formatting in docstring

b90ca00

pauljxtan mentioned this pull request Feb 24, 2020

Refactor remote job engine to Engine/Connection API #294

Merged

antalszava requested a review from jswinarton February 24, 2020 22:30

antalszava added review-ready API labels Feb 24, 2020

co9olguy requested changes Feb 24, 2020

View reviewed changes

antalszava and others added 3 commits February 24, 2020 18:08

Update tests/frontend/test_configuration.py

18a5d33

Co-Authored-By: Nathan Killoran <[email protected]>

Modifying docstrings based on comments

42071b0

Merge branch 'config_refactor' of https:/XanaduAI/strawbe…

ec800c3

…rryfields into config_refactor

antalszava requested a review from co9olguy February 25, 2020 00:24

josh146 requested changes Feb 25, 2020

View reviewed changes

antalszava added 4 commits February 25, 2020 12:47

Docstrings, renaming to load_config_file_if_found, adding defaults to…

1f04d5c

… module level attributes

Change function to get_config_filepath and adjust tests as well accor…

b82c60f

…dingly; change load_config using it

Renaming create_config

544b60c

Modifying tests to use setenv from monkeypatching in tests

228a0c4

co9olguy approved these changes Feb 25, 2020

View reviewed changes

strawberryfields/configuration.py Outdated Show resolved Hide resolved

Adding configuration options and environment variables to module docs…

4614d83

…tring, inserting refs to these in function docstrings, correcting Kwargs section naming in docstrings

antalszava force-pushed the config_refactor branch from 43d385d to 4614d83 Compare February 26, 2020 01:09

Removing redundant return None statement

09a3b3d

antalszava requested a review from josh146 February 26, 2020 01:15

josh146 approved these changes Feb 26, 2020

View reviewed changes

antalszava and others added 7 commits February 25, 2020 21:16

Remove update_config function, add keep_valid_options function and ad…

ef68ab5

…d tests

Addig DEFAULT_CONFIG_SPEC

3ca025e

Removed debug option for now

3772015

Update strawberryfields/configuration.py

d21f3fe

Co-Authored-By: Josh Izaac <[email protected]>

Update strawberryfields/configuration.py

7c73d5c

Co-Authored-By: Josh Izaac <[email protected]>

Moving configuration details to page, updating the configuration page…

47eac17

… with kwargs, adding port to file and env vars

Resolving conflicts

7bb97c8

josh146 approved these changes Feb 26, 2020

View reviewed changes

antalszava and others added 5 commits February 25, 2020 22:53

Update strawberryfields/configuration.py

e0938a9

Co-Authored-By: Josh Izaac <[email protected]>

Applying comments: Configuration page modification, removing redundan…

17be8bb

…t module attributes, reword module docstring

Merge branch 'config_refactor' of https:/XanaduAI/strawbe…

3d2aa7d

…rryfields into config_refactor

CHANGELOG

6635879

Merge branch 'demo' into config_refactor

0063663

antalszava merged commit 7a9ef04 into demo Feb 26, 2020

antalszava deleted the config_refactor branch February 26, 2020 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config refactor #298

Config refactor #298

antalszava commented Feb 21, 2020 •

edited

Loading

codecov bot commented Feb 21, 2020 •

edited

Loading

co9olguy left a comment

antalszava commented Feb 25, 2020

josh146 left a comment

josh146 Feb 25, 2020

antalszava Feb 26, 2020

josh146 Feb 25, 2020

antalszava Feb 25, 2020

josh146 Feb 25, 2020

antalszava Feb 25, 2020

josh146 Feb 26, 2020

josh146 Feb 25, 2020

co9olguy left a comment

antalszava commented Feb 26, 2020

josh146 left a comment

josh146 Feb 26, 2020

josh146 Feb 26, 2020

josh146 Feb 26, 2020

antalszava commented Feb 26, 2020

josh146 left a comment

josh146 Feb 26, 2020

antalszava Feb 26, 2020

josh146 Feb 26, 2020

antalszava Feb 26, 2020

josh146 Feb 26, 2020

antalszava Feb 26, 2020

josh146 Feb 26, 2020


		update_from_environment_variables(config)

		config_from_keyword_arguments = {"api": kwargs}

Config refactor #298

Config refactor #298

Conversation

antalszava commented Feb 21, 2020 • edited Loading

codecov bot commented Feb 21, 2020 • edited Loading

Codecov Report

co9olguy left a comment

Choose a reason for hiding this comment

antalszava commented Feb 25, 2020

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

co9olguy left a comment

Choose a reason for hiding this comment

antalszava commented Feb 26, 2020

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava commented Feb 26, 2020

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava commented Feb 21, 2020 •

edited

Loading

codecov bot commented Feb 21, 2020 •

edited

Loading