Skip to content

Dont write defaults to config #1188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
feast-ci-bot merged 8 commits into from
Nov 22, 2020
Merged

Dont write defaults to config #1188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
39cac68
S3 endpoint configuration #1169
mike0sv Nov 16, 2020
01def7f
Add allow_no_value=True to ConfigParser
mike0sv Nov 17, 2020
e8d8800
Merge branch 'master' into master_mike0sv
mike0sv Nov 20, 2020
6aa943f
New constants API
mike0sv Nov 20, 2020
caf1f6d
fix for other types of get
mike0sv Nov 20, 2020
ca3ce66
return to the old logic and some testing
mike0sv Nov 21, 2020
2684151
oooopsie
mike0sv Nov 21, 2020
9aaf593
clear
mike0sv Nov 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 29 additions & 29 deletions sdk/python/feast/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from feast.constants import ConfigOptions as opt

_logger = logging.getLogger(__name__)
_UNSET = object()

@woop woop Nov 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the object() type used here? Is there any reason why we can't use default=None?

@mike0sv mike0sv Nov 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a signleton, just like configparser._UNSET. If you use None, either you can't use None as default value, or you'll get None even if you don't want default value and do expect an exception



def _init_config(path: str):
Expand All @@ -50,17 +51,14 @@ def _init_config(path: str):
os.makedirs(os.path.dirname(config_dir), exist_ok=True)

# Create the configuration file itself
config = ConfigParser(defaults=opt().defaults())
config = ConfigParser(defaults=opt().defaults(), allow_no_value=True)
if os.path.exists(path):
config.read(path)

# Store all configuration in a single section
if not config.has_section(CONFIG_FILE_SECTION):
config.add_section(CONFIG_FILE_SECTION)

# Save the current configuration
config.write(open(path, "w"))

return config


Expand Down Expand Up @@ -117,69 +115,66 @@ def __init__(
self._config = config # type: ConfigParser
self._path = path # type: str

def get(self, option):
def _get(self, option, default, get_method):
fallback = {} if default is _UNSET else {"fallback": default}
return get_method(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
**fallback,
)

def get(self, option, default=_UNSET):

@woop woop Nov 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike0sv How are you planning for people to use this default argument? Do you want them to define defaults throughout the code base, for example .get(key, default="foo")?

@mike0sv mike0sv Nov 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, his option can be used if there is no value even in .constants. Also this allows to get arbitrary options from config (it's a side effect, so I don't know if cases where this is needed exist).
For now, if you try to get an option, which is declared as Optional in .constants (more precisely, that has None as default value), you'll get an error (because of the filtering in the defaults method)

"""
Returns a single configuration option as a string

Args:
option: Name of the option
default: Default value to return if option is not found

Returns: String option that is returned

"""
return self._config.get(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
)
return self._get(option, default, self._config.get)

def getboolean(self, option):
def getboolean(self, option, default=_UNSET):
"""
Returns a single configuration option as a boolean

Args:
option: Name of the option
default: Default value to return if option is not found

Returns: Boolean option value that is returned

"""
return self._config.getboolean(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
)
return self._get(option, default, self._config.getboolean)

def getint(self, option):
def getint(self, option, default=_UNSET):
"""
Returns a single configuration option as an integer

Args:
option: Name of the option
default: Default value to return if option is not found

Returns: Integer option value that is returned

"""
return self._config.getint(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
)
return self._get(option, default, self._config.getint)

def getfloat(self, option):
def getfloat(self, option, default=_UNSET):
"""
Returns a single configuration option as an integer

Args:
option: Name of the option
default: Default value to return if option is not found

Returns: Float option value that is returned

"""
return self._config.getfloat(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
)
return self._get(option, default, self._config.getfloat)

def set(self, option, value):
"""
Expand Down Expand Up @@ -211,7 +206,12 @@ def save(self):
Save the current configuration to disk. This does not include
environmental variables or initialized options
"""
self._config.write(open(self._path, "w"))
defaults = self._config.defaults()
try:
self._config._defaults = {}
self._config.write(open(self._path, "w"))
finally:
self._config._defaults = defaults

def __str__(self):
result = ""
Expand Down
21 changes: 21 additions & 0 deletions sdk/python/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@ def test_default_options(self):
config = Config(path=path)
assert config.get("CORE_URL") == "localhost:6565"

def test_defaults_are_not_written(self):
"""
default values are not written to config file
"""
fd, path = mkstemp()
config = Config(path=path)
config.set("option", "value")
config.save()
with open(path) as f:
assert f.read() == "[general]\noption = value\n\n"

def test_type_casting(self):
"""
Test type casting of strings to other types
Expand All @@ -117,6 +128,16 @@ def test_type_casting(self):
assert config.getfloat("FLOAT_VAR") == 1.0
assert config.getboolean("BOOLEAN_VAR") is True

def test_type_casting_of_defaults(self):
"""
default values are casted as expected
"""
fd, path = mkstemp()
config = Config(path=path)
assert isinstance(config.getboolean("enable_auth"), bool)
assert isinstance(config.getint("DATAPROC_EXECUTOR_INSTANCES"), int)
assert isinstance(config.getfloat("DATAPROC_EXECUTOR_INSTANCES"), float)

def test_set_value(self):
"""
Test type casting of strings to other types
Expand Down