Skip to content
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

Jump to bottom

Configure OQT using files or environment variables #255

Merged
merged 4 commits into from
Jul 21, 2022
Merged

Configure OQT using files or environment variables #255

merged 4 commits into from
Jul 21, 2022

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Feb 21, 2022 •
edited

Description

Configure OQT using files or environment variables.

All configuration can be done using environment variables.

All configuration can also be done by providing a configuration file (yaml format).
The default configuration file path is workers/config/config.yaml. The path to the configuration can be changed by setting the path in the environment variable OQT_CONFIG.
A sample configuration file is provided at workers/config/config.yaml.

Corresponding issue

  • None

Checklist

Open question:

  • Should configuration variables be made available as globals through definitions.py? -> No. Two functions to get either all config vars or a single value for a given key are provided.
  • Tests for config are very minmal. Should more time be spent to implement further tests?

@matthiasschaub matthiasschaub changed the title Config Add configuration of OQT using a file or environment variables Feb 21, 2022
@matthiasschaub matthiasschaub changed the title Add configuration of OQT using a file or environment variables Add configuration using a file or environment variables Feb 21, 2022
@matthiasschaub matthiasschaub linked an issue Mar 11, 2022 that may be closed by this pull request
Create indicators of a single class and a single layer for all features of a dataset #250
Closed
3 tasks
@matthiasschaub matthiasschaub added this to the Release 0.10.0 milestone Apr 13, 2022
@matthiasschaub matthiasschaub added the enhancement New feature or request label Apr 13, 2022
@matthiasschaub matthiasschaub self-assigned this Apr 13, 2022
@matthiasschaub matthiasschaub marked this pull request as ready for review April 26, 2022 07:16
@matthiasschaub matthiasschaub changed the title Add configuration using a file or environment variables Configure OQT using files or environment variables Apr 26, 2022
@matthiasschaub matthiasschaub added the CI Related to continuous integration (unit tests, build settings, etc.) label Apr 27, 2022
joker234
joker234 requested changes Apr 29, 2022
View reviewed changes
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have made some comments/questions, mostly about the details of the tests and the config design.

Two general remarks:

  1. Is there any config variable without a default and mandatory to be set by the user? If no, I would really prefer to not having the config file mandatory.
  2. The tests cannot depend on the local configuration. That doesn't make the tests independent, which they should be.

workers/ohsome_quality_analyst/config/sample.config.yaml Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/config/sample.config.yaml Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/config/sample.config.yaml Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
workers/Dockerfile Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/config/config.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_config.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_config.py Show resolved Hide resolved
workers/tests/unittests/test_definitions.py Show resolved Hide resolved
workers/tests/unittests/test_raster.py Outdated Show resolved Hide resolved
@matthiasschaub matthiasschaub mentioned this pull request May 2, 2022
Merged
5 tasks
@matthiasschaub matthiasschaub added the priority:high Should be addressed as soon as possible (next release) label May 10, 2022
@matthiasschaub matthiasschaub mentioned this pull request Jun 2, 2022
Merged
5 tasks
@matthiasschaub
Copy link
Collaborator Author

matthiasschaub commented Jun 22, 2022 •
edited

Thanks for the PR. I have made some comments/questions, mostly about the details of the tests and the config design.

Two general remarks:

1. Is there any config variable without a default and mandatory to be set by the user? If no, I would really prefer to not having the config file mandatory.

2. The tests cannot depend on the local configuration. That doesn't make the tests independent, which they should be.

Thanks for the throughout review so far!
After I addressed your review comments following statements about your general points are true:

  1. There is now mandatory config variable to be set and a config file is not mandatory
  2. The tests are not depending on a present config file anymore
    • At least this is true for the test_config.py file. For all other files a present configuration file will get used. Should this be avoided as well? My guess is no.

@matthiasschaub matthiasschaub added the waiting for review This pull request urgently needs a code review label Jul 18, 2022
@matthiasschaub matthiasschaub mentioned this pull request Jul 18, 2022
Merged
4 tasks
joker234
joker234 reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

Changelog missing!

workers/tests/unittests/test_raster.py Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/config/config.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/definitions.py Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_config.py Outdated Show resolved Hide resolved
joker234
joker234 reviewed Jul 21, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
workers/tests/unittests/test_config.py Outdated Show resolved Hide resolved
joker234
joker234 previously approved these changes Jul 21, 2022
View reviewed changes
@matthiasschaub
feat(config)!: config using files or envs
833d3ef 
Rewrite configuration by adding default configuration and the
possibility to configure OQT using a file or/and environment variables.

BREAKING: Rename env `OQT_SEMAPHORE` to `OQT_CONCURRENT_COMPUTATIONS
and `OHSOME_API` to `OQT_OHSOME_API`

Changes include:

- new module `config.py` located at root of the package
- move logging configuration to `config.py`
- configuration file path defaults to `workers/config/config.yaml`
- provide sample configuration at `workers/config/sample-config.yaml`
- move `definitions.py` to the root of the package
- update Dockerfile and docker-compose files
- update .gitignore
- update documentation
- update CHANGELOG
@matthiasschaub matthiasschaub merged commit 7371200 into main Jul 21, 2022
@matthiasschaub matthiasschaub deleted the config branch July 21, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joker234 joker234 joker234 approved these changes

Assignees

@matthiasschaub matthiasschaub

Labels
CI Related to continuous integration (unit tests, build settings, etc.) enhancement New feature or request priority:high Should be addressed as soon as possible (next release) waiting for review This pull request urgently needs a code review
Projects
None yet
Milestone
Release 0.11.0
Development

Successfully merging this pull request may close these issues.

Create indicators of a single class and a single layer for all features of a dataset
2 participants
@matthiasschaub @joker234