-
Notifications
You must be signed in to change notification settings - Fork 28
XP-456: replace CLI argument with a config file #221
XP-456: replace CLI argument with a config file #221
Conversation
202bd14
to
f4439b6
Compare
f4439b6
to
21d6017
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me so far. I will tests it tomorrow.
f48ced1
to
f93911d
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create follow-up tickets to make changes to docker files and any other changes. Would be great to have only changes about configuration in this pr and everything else in other dedicated prs. Also it’s good for history of tickets and changes introduced to keep the scope according to DoD of linked jira ticket.
f77ee94
to
6a90382
Compare
This comment has been minimized.
This comment has been minimized.
6a90382
to
d03367a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d03367a
to
885e505
Compare
cd2bf5b
to
d17306b
Compare
Thanks for the review @finiteprods, I applied the changes you suggested. |
description (str): description of the expected type of value | ||
for this configuration item | ||
""" | ||
return f"Invalid `{key}`: value must be {description}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this function after updating structlog message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atymoshchuk sorry, I'm not sure what you mean here.
ab5839f
to
96c9721
Compare
430f8c6
to
fdcf7de
Compare
This conflicts with #237 but I'll rebase after the final review. |
854b721
to
1751f9c
Compare
05166a1
to
326f8e3
Compare
Cc: finiteprods <kwok.doc@gmail.com> Cc: Robert Steiner <robertt.debug@gmail.com> Cc: janpetschexain <58227040+janpetschexain@users.noreply.github.com> References ========== https://xainag.atlassian.net/browse/XP-456 https://xainag.atlassian.net/browse/DO-58 Rationale ========= The CLI is getting complex so it is worth loading the configuration from a file instead. Implementation details ====================== TOML ---- We decided to use TOML for the following reasons: - it is human friendly, ie easy to read and write - our configuration has a pretty flat structure which makes TOML quite adapted - it is well specified and has lots of implementation - it is well known The other options we considered: - INI: it is quite frequent in the Python ecosystem to use INI for config files, and the standard library even provides support for this. However, INI is not as powerful as TOML and does not have a specification - JSON: it is very popular but is not human friendly. For instance, it does not support comments, is very verbose, and breaks easily (if a trailing comma is forgotten at the end of a list for instance) - YAML: another popular choice, but is in my opinion more complex than TOML. Validation ---------- We use the third-party `schema` library to validate the configuration. It provides a convenient way to: - declare a schema to validate our config - leverage third-party libraries to validate some inputs (we use the `idna` library to validate hostnames) - define our own validators - transform data after it has been validated: this can be useful to turn a relative path into an absolute one for example - provide user friendly error message when the configuration is invalid The `Config` class ------------------ By default, the `schema` library returns a dictionary containing a valid configuration, but that is not convenient to manipulate in Python. Therefore, we dynamically create a `Config` class from the configuration schema, and instantiate a `Config` object from the data returned by the `schema` validator. Package re-organization ----------------------- We moved the command line and config file logic into its own `config` sub-package, and moved the former `xain_fl.cli.main` entrypoint into the `xain_fl.__main__` module. Docker infrastructure --------------------- - Cache the xain_fl dependencies. This considerably reduces "edit->build-> debug" cycle, since installing the dependencies takes about 30 minutes. - Move all the docker related files into the `docker/` directory Current limitations and future work ----------------------------------- 1. The documentation generated for the `ServerConfig`, `AiConfig` and `StorageConfig` classes is wrong. Each attribute is documented as "Alias for field number X". This can be fixed by having `create_class_from_schema()` setting the `__doc__` attribute for each attribute. However, we won't be able to automatically document the type of each attribute. 2. When the configuration contains an invalid value, the error message we generate does not contain the invalid value in question. I think it is possible to enable this in the future but haven't really looked into it.
326f8e3
to
06ba121
Compare
Summary: fix `coordinator` command in the docker "release" image. References: xaynetwork#221 https://xainag.atlassian.net/browse/XP-456
Summary: fix `coordinator` command in the docker "release" image. References: #221 https://xainag.atlassian.net/browse/XP-456
Summary
replace the CLI arguments with a config file
Reference:
https://xainag.atlassian.net/browse/XP-456
https://xainag.atlassian.net/browse/XP-512
Rationale
The CLI is getting complex so it is worth loading the configuration
from a file instead.
Implementation details
TOML
We decided to use TOML for the following reasons:
quite adapted
The other options we considered:
config files, and the standard library even provides support for
this. However, INI is not as powerful as TOML and does not have a
specification
it does not support comments, is very verbose, and breaks
easily (if a trailing comma is forgotten at the end of a list for
instance)
than TOML.
Validation
We use the third-party
schema
library to validate theconfiguration. It provides a convenient way to:
idna
library to validate hostnames)turn a relative path into an absolute one for example
invalid
The
Config
classBy default, the
schema
library returns a dictionary containing avalid configuration, but that is not convenient to manipulate in
Python. Therefore, we dynamically create a
Config
class from theconfiguration schema, and instantiate a
Config
object from the datareturned by the
schema
validator.Package re-organization
We moved the command line and config file logic into its own
config
sub-package, and moved the former
xain_fl.cli.main
entrypoint intothe
xain_fl.__main__
module.Docker infrastructure
"edit->build-> debug" cycle, since installing the dependencies takes
about 30 minutes.
docker/
directoryLimitations and future work
The documentation generated for the
ServerConfig
,AiConfig
andStorageConfig
classes is wrong. Each attribute isdocumented as "Alias for field number X". This can be fixed by
having
create_class_from_schema()
setting the__doc__
attributefor each attribute. However, we won't be able to automatically
document the type of each attribute.
When the configuration contains an invalid value, the error message
we generate does not contain the invalid value in question. I think
it is possible to enable this in the future but haven't really
looked into it.
Reviewer checklist
Reviewer agreement:
Merge request checklist
XP-XXX <a description in imperative form>
.XP-XXX <a description in imperative form>
.Code checklist
XP-XXX-<a_small_stub>
.