Sxnan commented on code in PR #122: URL: https://github.com/apache/flink-agents/pull/122#discussion_r2287466823
########## python/flink_agents/runtime/local_configuration.py: ########## @@ -0,0 +1,225 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +from pathlib import Path +from typing import Any, ClassVar, Dict, Optional + +import yaml +from pyflink.common import Configuration + +from flink_agents.api.configuration import ( + ConfigOption, + ReadableConfiguration, + WritableConfiguration, +) + + +def flatten_dict(d: Dict, parent_key: str = '', sep: str = '.') -> Dict[str, Any]: + """Flatten a nested dictionary into a single-level dictionary. + + This function recursively traverses the dictionary, converting multi-level + nested key-value pairs into a single-level structure, where nested levels + are represented by joining key names with the specified separator. + + Args: + d (Dict): The nested dictionary to be flattened + parent_key (str): The parent key name, used in recursion to track the + upper-level key path. Defaults to an empty string. + sep (str): The separator used to join parent and child keys. + Defaults to dot ('.'). + + Returns: + Dict[str, Any]: A flattened single-level dictionary where keys from + the original nested structure are joined with the separator + """ + items = {} + for k, v in d.items(): + new_key = f"{parent_key}{sep}{k}" if parent_key else k + if isinstance(v, dict): + items.update(flatten_dict(v, new_key, sep=sep)) + else: + items[new_key] = v + return items + +class LocalConfiguration(WritableConfiguration, ReadableConfiguration): + """Base class for config objects in the system. + Provides a flat dict interface to access nested config values. + """ + + _conf_data: ClassVar[dict[str, Any]] = {} Review Comment: Why is this a class variable instead of a instance variable? ########## python/flink_agents/examples/chat_ollama_exmaple.py: ########## @@ -39,7 +39,6 @@ model = os.environ.get("OLLAMA_CHAT_MODEL", "qwen3:8b") - Review Comment: nit: unnecessary change ########## python/flink_agents/runtime/local_configuration.py: ########## @@ -0,0 +1,225 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +from pathlib import Path +from typing import Any, ClassVar, Dict, Optional + +import yaml +from pyflink.common import Configuration + +from flink_agents.api.configuration import ( + ConfigOption, + ReadableConfiguration, + WritableConfiguration, +) + + +def flatten_dict(d: Dict, parent_key: str = '', sep: str = '.') -> Dict[str, Any]: + """Flatten a nested dictionary into a single-level dictionary. + + This function recursively traverses the dictionary, converting multi-level + nested key-value pairs into a single-level structure, where nested levels + are represented by joining key names with the specified separator. + + Args: + d (Dict): The nested dictionary to be flattened + parent_key (str): The parent key name, used in recursion to track the + upper-level key path. Defaults to an empty string. + sep (str): The separator used to join parent and child keys. + Defaults to dot ('.'). + + Returns: + Dict[str, Any]: A flattened single-level dictionary where keys from + the original nested structure are joined with the separator + """ + items = {} + for k, v in d.items(): + new_key = f"{parent_key}{sep}{k}" if parent_key else k + if isinstance(v, dict): + items.update(flatten_dict(v, new_key, sep=sep)) + else: + items[new_key] = v + return items + +class LocalConfiguration(WritableConfiguration, ReadableConfiguration): + """Base class for config objects in the system. + Provides a flat dict interface to access nested config values. + """ + + _conf_data: ClassVar[dict[str, Any]] = {} + + def get_int(self, key: str, default: int=0) -> int: Review Comment: The default value of an int configuration is not always 0. Similar to other get methods. It is more intuitive to throw an exception if `get_xxx` method is called when the key is missing and the default value is not given. ########## python/flink_agents/examples/chat_ollama_exmaple.py: ########## @@ -58,7 +57,6 @@ def math_chat_model() -> Tuple[Type[BaseChatModelSetup], Dict[str, Any]]: """ChatModel which focus on math, and reuse ChatModelServer.""" return OllamaChatModelSetup, { "name": "math_chat_model", - "connection": "ollama_server", Review Comment: Why do we remove this? ########## python/flink_agents/api/configuration.py: ########## @@ -0,0 +1,148 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +from abc import ABC, abstractmethod +from typing import Any, Type + + +class ConfigOption: + """A configuration option defines a configuration key with its type and default + value. + + Args: + key: The configuration key name + type: The expected type of the configuration value (int, float, str, or bool) + default: The default value for this configuration option + """ + + def __init__(self, key: str, type: Type[Any], default: Any) -> None: Review Comment: We should support a config option without a default value, which means that the ConfigOption is not optional and we should throw an exception if it is missing. ########## python/requirements/test_requirements.txt: ########## @@ -17,3 +17,4 @@ setuptools>=75.3 pytest==8.4.0 pydantic==2.11.4 apache-flink==1.20.1 +pyyaml==6.0.2 Review Comment: Why do we maintain two sets of requirements? One in the `pyproject.toml` and one in the requirements folder? And they are inconsistent already. ########## python/flink_agents/plan/agent_plan.py: ########## @@ -229,35 +229,51 @@ def _get_actions(agent: Agent) -> List[Action]: return actions -def _get_resource_providers(agent: Agent) -> List[ResourceProvider]: +def _get_resource_providers(agent: Agent, config_data: Dict[str, Any]) -> List[ResourceProvider]: resource_providers = [] for name, value in agent.__class__.__dict__.items(): if hasattr(value, "_is_chat_model"): if isinstance(value, staticmethod): value = value.__func__ if callable(value): + final_kwargs = {} clazz, kwargs = value() + resource_kwargs = _get_module_conf(config_data, name) + clazz_kwargs = _get_module_conf(config_data, clazz.__name__) + + final_kwargs.update(clazz_kwargs) Review Comment: Instead of injecting the configuration in the kwargs, it is better to pass the configuration to the resource provider. Then, the resource provider will pass the configuration to the Resource when creating the Resource. The Resource implementation will decide how to use the configuration properly. ########## api/src/main/java/org/apache/flink/agents/api/FlinkAgentsCoreOptions.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.agents.api; + +import org.apache.flink.configuration.ConfigOption; +import org.apache.flink.configuration.ConfigOptions; + +public final class FlinkAgentsCoreOptions { + public static final ConfigOption<String> EXAMPLE_OPTION = Review Comment: Can we think of a better config option to showcase? Adding a temporary field to a public API class may not be a good idea. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
