You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@otava.apache.org by "smccarthy788 (via GitHub)" <gi...@apache.org> on 2025/08/09 18:55:58 UTC
[PR] OTAVA-82: use ConfigArgParse to create Config [otava]
smccarthy788 opened a new pull request, #86:
URL: https://github.com/apache/otava/pull/86
See #82.
Took a stab at this. I fear I've misunderstood the goal:
> use ConfigArgParse and support all options in the same library. (CLI options, yaml file, [env vars])
I've pushed the entire creation of the config object through ConfigArgParse. It feels a bit unwieldy, or... square peg through a round hole? So I'm not super attached to this implementation. Still, putting it up for consideration / reference.
Pros:
- all config options are available to be set in config file, env var, or CLI (with precedence as desired)
- good looking documentation of the CLI arguments & env vars output via `otava --help`:
<details>
<summary>Help Output</summary>
```
$ uv run otava --help
usage: otava [-h] [--config-file CONFIG_FILE] [--graphite-url GRAPHITE_URL] [--grafana-url GRAFANA_URL] [--grafana-user GRAFANA_USER] [--grafana-password GRAFANA_PASSWORD] [--slack-token SLACK_TOKEN] [--postgres-hostname POSTGRES_HOSTNAME]
[--postgres-port POSTGRES_PORT] [--postgres-username POSTGRES_USERNAME] [--postgres-password POSTGRES_PASSWORD] [--postgres-database POSTGRES_DATABASE] [--bigquery-project BIGQUERY_PROJECT] [--bigquery-dataset BIGQUERY_DATASET]
[--bigquery-credentials BIGQUERY_CREDENTIALS]
{list-tests,list-metrics,list-groups,analyze,regressions,remove-annotations,validate} ...
Hunts performance regressions in Fallout results
positional arguments:
{list-tests,list-metrics,list-groups,analyze,regressions,remove-annotations,validate}
list-tests list available tests
list-metrics list available metrics for a test
list-groups list available groups of tests
analyze analyze performance test results
regressions find performance regressions
validate validates the tests and metrics defined in the configuration
optional arguments:
-h, --help show this help message and exit
--config-file CONFIG_FILE
Otava config file path [env var: OTAVA_CONFIG]
Graphite Options:
Options for Graphite configuration
--graphite-url GRAPHITE_URL
Graphite server URL [env var: GRAPHITE_ADDRESS]
Grafana Options:
Options for Grafana configuration
--grafana-url GRAFANA_URL
Grafana server URL [env var: GRAFANA_ADDRESS]
--grafana-user GRAFANA_USER
Grafana server user [env var: GRAFANA_USER]
--grafana-password GRAFANA_PASSWORD
Grafana server password [env var: GRAFANA_PASSWORD]
Slack Options:
Options for Slack configuration
--slack-token SLACK_TOKEN
Slack bot token to use for sending notifications [env var: SLACK_BOT_TOKEN]
Postgres Options:
Options for Postgres configuration
--postgres-hostname POSTGRES_HOSTNAME
PostgreSQL server hostname [env var: POSTGRES_HOSTNAME]
--postgres-port POSTGRES_PORT
PostgreSQL server port [env var: POSTGRES_PORT]
--postgres-username POSTGRES_USERNAME
PostgreSQL username [env var: POSTGRES_USERNAME]
--postgres-password POSTGRES_PASSWORD
PostgreSQL password [env var: POSTGRES_PASSWORD]
--postgres-database POSTGRES_DATABASE
PostgreSQL database name [env var: POSTGRES_DATABASE]
BigQuery Options:
Options for BigQuery configuration
--bigquery-project BIGQUERY_PROJECT
BigQuery project ID [env var: BIGQUERY_PROJECT_ID]
--bigquery-dataset BIGQUERY_DATASET
BigQuery dataset [env var: BIGQUERY_DATASET]
--bigquery-credentials BIGQUERY_CREDENTIALS
BigQuery credentials file [env var: BIGQUERY_VAULT_SECRET]
Args that start with '--' can also be set in a config file (specified via --config-file). Config file syntax allows: key=value, flag=true, stuff=[a,b,c] (for details, see syntax at https://goo.gl/R74nmi). In general, command-line values
override environment variables which override config file values which override defaults.
```
</details>
Cons:
- custom parser to handle nested config objects
- abstraction leaking into the config objects; config objects now need to know about configargparse.
- need to use `parse_known_args` to avoid exceptions from parts of the config we don't want to include in the CLI (tests, templates, test_groups)
- creating a Config object programatically from a configuration file is... _weird_ (although I avoided exposing people to this by adding a helper function):
```python
config_file = Path("tests/resources/substitution_test_config.yaml")
parser = create_config_parser(config_file)
args, unknown_args = parser.parse_known_args(args=[])
config = load_config_from_parser_args(args, config_file)
```
If we do want to pursue this implementation, I'll flesh out the unit tests more. So far I've only added enough to validate the basics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "smccarthy788 (via GitHub)" <gi...@apache.org>.
smccarthy788 commented on PR #86:
URL: https://github.com/apache/otava/pull/86#issuecomment-3201273482
> For the nested settings, I would say don't be afraid to just flatten or otherwise change the structure of the config file.
Personally I quite like the nested structure of the config. IMO it's worth the custom parser.
> I think there was something in the yaml configuration files - maybe it was the template system and how you configured each test... Where I felt that is probably best left as it is, "yaml only". I don't see it in this patch, so maybe that's because you likewise left those out of scope for this patch?
I haven't touched how the `tests` are generated from the `templates` (I think this is what you're referring to). Still "yaml only", even more so now that I've removed the `expandvars` (no more arbitrary env vars).
I have an itch to separate those from the config, but that would probably be best as a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "Gerrrr (via GitHub)" <gi...@apache.org>.
Gerrrr commented on PR #86:
URL: https://github.com/apache/otava/pull/86#issuecomment-3202307730
I will only be able to review this PR closer to the end of next week. Since @henrikingo reviewed it, there is no need to wait for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "smccarthy788 (via GitHub)" <gi...@apache.org>.
smccarthy788 merged PR #86:
URL: https://github.com/apache/otava/pull/86
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on code in PR #86:
URL: https://github.com/apache/otava/pull/86#discussion_r2265255543
##########
otava/main.py:
##########
@@ -15,21 +15,21 @@
# specific language governing permissions and limitations
# under the License.
-import argparse
import copy
import logging
import sys
from dataclasses import dataclass
from datetime import datetime, timedelta
from typing import Dict, List, Optional
+import configargparse
Review Comment:
Actually it is common to `import configargparse as argparse` to avoid having to change the rest of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "smccarthy788 (via GitHub)" <gi...@apache.org>.
smccarthy788 commented on PR #86:
URL: https://github.com/apache/otava/pull/86#issuecomment-3201528323
@henrikingo I think this is ready for another round of review. @Gerrrr I'd appreciate your feedback too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OTAVA-82: use ConfigArgParse to create Config [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on PR #86:
URL: https://github.com/apache/otava/pull/86#issuecomment-3203914581
It's allowed to review also after merging. But this is fairly straightforward adoption of configargparse. Good work Sean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org