-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Json configuration #141
base: main
Are you sure you want to change the base?
Json configuration #141
Conversation
ba2ca23
to
34f4ceb
Compare
config.json can not be used in a package directory, even if it would be part of the package as the package directory is usually read only for a user. IMHO provide both options and make the command line override the ENV var. To be super user friendly the server may serve a default config.json file to be saved and adapted by the user. Just an idea. |
34f4ceb
to
7f0b8bb
Compare
I now made a lot of changes :D
|
e243d62
to
3aa48d6
Compare
) | ||
|
||
|
||
def test_config() -> None: |
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.
This test will ensure, that the default config is always parseable.
@@ -0,0 +1,9 @@ | |||
{ | |||
"prediction_hours": 48, |
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.
I haven't set the pydantic validation to strict so "48"
would be valid, too.
c25e9b0
to
2f85a6b
Compare
src/akkudoktoreos/config.py
Outdated
def main() -> None: | ||
# Update to pydantic v2 when possible | ||
# https://github.com/SupImDos/pydantic-argparse/issues/56 | ||
parser = pydantic_argparse.ArgumentParser( |
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.
this is sick
2f85a6b
to
acf1022
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.
Pretty cool PR! It would be nice if all the configuration settings are somewhere documented in the README. Does this support all the config values as env as well? If so that would be really nice for a Docker setup.
I didn´t read all parts of the code yet, however one more config variable is probably cache directory, maybe output file (generated pdf), and if #129 gets merged output_directory.
requirements.txt
Outdated
@@ -9,3 +9,5 @@ requests==2.32.3 | |||
pytest==8.3.3 | |||
pytest-cov==5.0.0 | |||
pandas==2.2.3 | |||
pydantic==1.10.18 |
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.
Could you make it work with pydantic 2 as well? - Just saw your comment with argparse. Is there another parser available with pydantic v2? If we switch framework from flask to fastapi, pydantic v2 would be neat.
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.
I am investigating a bit and found similar modules to pydantic-argparse. But I am not convinced, yet. I hope, that there will be an update, as some forks already support pydantic v2 and the maintainer is aware of this topic.
Otherwise I simplify the cli and remove the pydantic-argparse dependency for now. Is the switch already planned?
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.
I provided a draft here: #163 FastAPI would also work with pydantic v1, but why use a deprecated version if we can avoid it.
In the first draft I would like to keep the changes to the switch from "config.py" to "config.json". Further extensions should be done in a separate PR. |
acf1022
to
0dc84dd
Compare
Ready for testing? |
The base functionality is ready. I only want to change the CLI handling so that pydanticV2 can be used. |
0dc84dd
to
eb41091
Compare
Ready! I removed the CLI for now. I would do this in a proper way if it is desired: #155 |
5ad484d
to
c35e0a0
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.
To include default.config.json into the package please add to pyproject.toml:
[tool.setuptools.package-data]
akkudoktoreos = ["*.json", ] <--- add this
akkudoktoreosserver = ["data/*.npz", ]
You may see whats packaged by:
make dist
- create necessary folders - enforce that folders are created
c35e0a0
to
854f36c
Compare
|
Please edit docs/develop/getting_started.rst to reflect your changes. I know it is already outdated, it was just added as a placeholder for changes like this one. You may see your changes by: make docs
.venv/bin/python -m webbrowser build/docs/html/index.html |
I already adjusted the readme. In my opinion there shouldn't be redundant information as it is difficult to keep track of changes. Which file is prefered? Or what is the purpose of each file? |
The README is the landing page for the GitHub repo. getting_started.rst is part of the documentation that covers the total package in a lot more detail. |
You may improve the documentation of your config.py module. Here is what ChatGPT "thinks" of config.py: """
This module provides functionality to manage and handle configuration for the EOS system,
including loading, merging, and validating JSON configuration files. It also provides
utility functions for working directory setup and date handling.
Key features:
- Loading and merging configurations from default or custom JSON files
- Validating configurations using Pydantic models
- Managing directory setups for the application
- Utility to get prediction start and end dates
"""
import json
import os
import shutil
from datetime import datetime, timedelta
from pathlib import Path
from typing import Any, Optional
from pydantic import BaseModel, ValidationError
# Constants
EOS_DIR = "EOS_DIR"
ENCODING = "UTF-8"
CONFIG_FILE_NAME = "EOS.config.json"
DEFAULT_CONFIG_FILE = Path(__file__).parent.joinpath("default.config.json")
class FolderConfig(BaseModel):
"""
Folder configuration for the EOS system.
Attributes:
output (str): Directory path for output files.
cache (str): Directory path for cache files.
"""
output: str
cache: str
class EOSConfig(BaseModel):
"""
EOS system-specific configuration.
Attributes:
prediction_hours (int): Number of hours for predictions.
optimization_hours (int): Number of hours for optimizations.
penalty (int): Penalty factor used in optimization.
available_charging_rates_in_percentage (list[float]): List of available charging rates as percentages.
"""
prediction_hours: int
optimization_hours: int
penalty: int
available_charging_rates_in_percentage: list[float]
class BaseConfig(BaseModel):
"""
Base configuration for the EOS system.
Attributes:
directories (FolderConfig): Configuration for directory paths (output, cache).
eos (EOSConfig): Configuration for EOS-specific settings.
"""
directories: FolderConfig
eos: EOSConfig
class AppConfig(BaseConfig):
"""
Application-level configuration that extends the base configuration with a working directory.
Attributes:
working_dir (Path): The root directory for the application.
"""
working_dir: Path
def run_setup(self) -> None:
"""
Runs setup for the application by ensuring that required directories exist.
If a directory does not exist, it is created.
Raises:
OSError: If directories cannot be created.
"""
print("Checking directory settings and creating missing directories...")
for key, value in self.directories.model_dump().items():
if not isinstance(value, str):
continue
path = self.working_dir / value
if path.is_dir():
print(f"'{key}': {path}")
continue
print(f"Creating directory '{key}': {path}")
os.makedirs(path, exist_ok=True)
class SetupIncomplete(Exception):
"""
Exception class for errors related to incomplete setup of the EOS system.
"""
pass
def _load_json(path: Path) -> dict[str, Any]:
"""
Load a JSON file from a given path.
Args:
path (Path): Path to the JSON file.
Returns:
dict[str, Any]: Parsed JSON content.
Raises:
FileNotFoundError: If the JSON file does not exist.
json.JSONDecodeError: If the file cannot be parsed as valid JSON.
"""
with path.open("r") as f_in:
return json.load(f_in)
def _merge_json(default_data: dict[str, Any], custom_data: dict[str, Any]) -> dict[str, Any]:
"""
Recursively merge two dictionaries, using values from `custom_data` when available.
Args:
default_data (dict[str, Any]): The default configuration values.
custom_data (dict[str, Any]): The custom configuration values.
Returns:
dict[str, Any]: Merged configuration data.
"""
merged_data = {}
for key, default_value in default_data.items():
if key in custom_data:
custom_value = custom_data[key]
if isinstance(default_value, dict) and isinstance(custom_value, dict):
merged_data[key] = _merge_json(default_value, custom_value)
elif type(default_value) is type(custom_value):
merged_data[key] = custom_value
else:
merged_data[key] = default_value # Use default value if types differ
else:
merged_data[key] = default_value
return merged_data
def _config_update_available(merged_data: dict[str, Any], custom_data: dict[str, Any]) -> bool:
"""
Check if the configuration needs to be updated by comparing merged data and custom data.
Args:
merged_data (dict[str, Any]): The merged configuration data.
custom_data (dict[str, Any]): The custom configuration data.
Returns:
bool: True if there is a difference indicating that an update is needed, otherwise False.
"""
if merged_data.keys() != custom_data.keys():
return True
for key in merged_data:
value1 = merged_data[key]
value2 = custom_data[key]
if isinstance(value1, dict) and isinstance(value2, dict):
if _config_update_available(value1, value2):
return True
elif value1 != value2:
return True
return False
def get_config_file(path: Path, copy_default: bool) -> Path:
"""
Get the valid configuration file path. If the custom config is not found, it uses the default config.
Args:
path (Path): Path to the working directory.
copy_default (bool): If True, copy the default configuration if custom config is not found.
Returns:
Path: Path to the valid configuration file.
"""
config = path.resolve() / CONFIG_FILE_NAME
if config.is_file():
print(f"Using configuration from: {config}")
return config
if not path.is_dir():
print(f"Path does not exist: {path}. Using default configuration...")
return DEFAULT_CONFIG_FILE
if not copy_default:
print("No custom configuration provided. Using default configuration...")
return DEFAULT_CONFIG_FILE
try:
return Path(shutil.copy2(DEFAULT_CONFIG_FILE, config))
except Exception as exc:
print(f"Could not copy default config: {exc}. Using default copy...")
return DEFAULT_CONFIG_FILE
def _merge_and_update(custom_config: Path, update_outdated: bool = False) -> bool:
"""
Merge custom and default configurations, and optionally update the custom config if outdated.
Args:
custom_config (Path): Path to the custom configuration file.
update_outdated (bool): If True, update the custom config if it is outdated.
Returns:
bool: True if the custom config was updated, otherwise False.
"""
if custom_config == DEFAULT_CONFIG_FILE:
return False
default_data = _load_json(DEFAULT_CONFIG_FILE)
custom_data = _load_json(custom_config)
merged_data = _merge_json(default_data, custom_data)
if not _config_update_available(merged_data, custom_data):
print(f"Custom config {custom_config} is up-to-date...")
return False
print(f"Custom config {custom_config} is outdated...")
if update_outdated:
with custom_config.open("w") as f_out:
json.dump(merged_data, f_out, indent=2)
return True
return False
def load_config(
working_dir: Path, copy_default: bool = False, update_outdated: bool = True
) -> AppConfig:
"""
Load the application configuration from the specified directory, merging with defaults if needed.
Args:
working_dir (Path): Path to the working directory.
copy_default (bool): Whether to copy the default configuration if custom config is missing.
update_outdated (bool): Whether to update outdated custom configuration.
Returns:
AppConfig: Loaded application configuration.
Raises:
ValueError: If the configuration is incomplete or not valid.
"""
working_dir = working_dir.resolve()
config = get_config_file(working_dir, copy_default)
_merge_and_update(config, update_outdated)
with config.open("r", encoding=ENCODING) as f_in:
try:
base_config = BaseConfig.model_validate(json.load(f_in))
return AppConfig.model_validate(
{"working_dir": working_dir, **base_config.model_dump()}
)
except ValidationError as exc:
raise ValueError(f"Configuration {config} is incomplete or not valid: {exc}")
def get_working_dir() -> Path:
"""
Get the working directory for the application, either from an environment variable or the current working directory.
Returns:
Path: The path to the working directory.
"""
custom_dir = os.getenv(EOS_DIR)
if custom_dir is None:
working_dir = Path.cwd()
print(f"No custom directory provided. Setting working directory to: {working_dir}")
else:
working_dir = Path(custom_dir).resolve()
print(f"Custom directory provided. Setting working directory to: {working_dir}")
return working_dir
def get_start_enddate(
prediction_hours: int, startdate: Optional[datetime] = None
) -> tuple[str, str]:
"""
Calculate the start and end dates based on the given prediction hours and optional start date.
Args:
prediction_hours (int): Number of hours for predictions.
startdate (Optional[datetime]): Optional starting datetime.
Returns:
tuple[str, str]: The current date (start date) and end date in the format 'YYYY-MM-DD'.
"""
if startdate is None:
date = (datetime.now().date() + timedelta(hours=prediction_hours)).strftime("%Y-%m-%d")
date_now = datetime.now().strftime("%Y-%m-%d")
else:
date = (startdate + timedelta(hours=prediction_hours)).strftime("%Y-%m-%d")
date_now = startdate.strftime("%Y-%m-%d")
return date_now, date |
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.
Pretty cool you made it work with pydantic v2! For now you ommited the cli parsing / env support?
self.cache_time_file = os.path.join(self.cache_dir, "cache_timestamp.txt") | ||
self.prices = self.load_data(source) | ||
def __init__(self, source: str | Path, config: AppConfig, charges=0.000228): # 228 | ||
self._cache_dir = config.working_dir / config.directories.cache |
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.
For permanent installation it would be nice to have free choice over the directory structure. Support for absolute paths would be great (maybe build the full path directly in AppConfig)
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.
for this PR I just wanted to replace the current functionality. And would it really be needed to have different paths? They are all set from cwd or the provided user path.
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.
Fair point for a different PR. I'm thinking of Linux directory structure that splits up a package across the file system.
self.prices = self.load_data(source) | ||
def __init__(self, source: str | Path, config: AppConfig, charges=0.000228): # 228 | ||
self._cache_dir = config.working_dir / config.directories.cache | ||
if not self._cache_dir.is_dir(): |
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.
Since we create the directories at app startup, I would omit this check.
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.
the class could be used on it's own.That's why I put this early fail check
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.
I would argue since this is a server application after all, we can probably assume that the server startup is the only entrance point.
src/akkudoktoreos/config.py
Outdated
if not isinstance(value, str): | ||
continue | ||
path = self.working_dir / value | ||
if path.is_dir(): |
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.
Should probably omit this check as makedirs will be fine with directory (and maybe fail if it would be a file).
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.
I put this check to differentiate the print message. But I can reduce the code and just print the folder
The env (var?) support is provided and also tested in one test. CLI would be the next PR. |
24d8b26
to
a42b7ac
Compare
Please re-run all tests |
Add a json configuration.
To discuss/questions:
TODO