wenjin272 commented on code in PR #1223:
URL: https://github.com/apache/flink-agents/pull/1223#discussion_r4244289295
##########
python/flink_agents/runtime/java_resource_adapter.py:
##########
@@ -56,6 +56,10 @@ def getSkillDirs(self, skill_names: list[str]) -> list[str]:
"""Resolve the directories of Java-hosted skills."""
return list(self._bridge.getSkillDirs(skill_names) or [])
+ def resolveSkillNamesForSources(self, sources: list[dict]) -> list[str]:
+ """Resolve the names of Java-hosted skills declared by source specs."""
+ return list(self._bridge.resolveSkillNamesForSources(sources) or [])
Review Comment:
The actual bridge here is Java `PythonResourceAdapterImpl`, but
`resolveSkillNamesForSources()` was added to `JavaResourceAdapter`. A Java
agent using a Python model with constructor-provided `Skills` therefore raises
`AttributeError`, confirmed through real Pemja. Could we add the missing
delegation and test the actual bridge?
##########
docs/content/docs/development/react_agent.md:
##########
@@ -332,3 +332,36 @@ RowTypeInfo myRowTypeInfo =
{{< /tabs >}}
+### Skills
+User can pass the names of [skills]({{< ref "docs/development/skills" >}}) the
agent may use directly to the constructor, instead of adding a `skills`
argument to the chat model descriptor.
+
+{{< tabs "Skills" >}}
+
+{{< tab "Python" >}}
+```python
+my_react_agent = ReActAgent(
+ chat_model=chat_model_descriptor,
+ prompt=my_prompt,
+ skills=["math-calculator"],
+)
+```
+{{< /tab >}}
+
+{{< tab "Java" >}}
+```java
+ReActAgent myReActAgent =
+ new ReActAgent(chatModelDescriptor, myPrompt, null,
List.of("math-calculator"));
+```
+{{< /tab >}}
+
+{{< /tabs >}}
+
+The constructor skills are combined with any `skills` already set on the chat
model descriptor:
+
+- The descriptor's skills come first, followed by the constructor skills; a
name listed more than once is kept only at its first occurrence.
+- Passing no skills, or an empty list, adds nothing: the descriptor is used
exactly as given.
+- The descriptor you pass in is not modified; the agent uses a copy with the
combined skills.
+- Only skill names are combined. The command policy of the built-in `bash`
tool, such as `allowed_commands`, is always taken from the chat model
descriptor, so set it there.
+
+The skill sources must still be registered on the execution environment with
`add_resource(..., ResourceType.SKILLS, ...)`.
Review Comment:
Could we also document the `Skills`-object constructor with Java/Python
examples? This section only describes skill-name lists and says sources must be
registered on the environment, but passing a `Skills` object registers its
sources on the agent automatically. Please distinguish the two forms and
document how source-derived names combine with explicit descriptor skills,
including the annotation → environment → agent precedence.
##########
api/src/main/java/org/apache/flink/agents/api/agents/Agent.java:
##########
@@ -101,10 +100,22 @@ public Agent addAction(
return this;
}
+ /**
+ * Merge environment-level resources into this agent. Entries already
present at the agent level
+ * are kept (agent wins on duplicates). Environment entries are placed
before agent entries in
+ * iteration order, so downstream merging sees env-then-agent precedence.
+ */
public void addResourcesIfAbsent(Map<ResourceType, Map<String, Object>>
resources) {
for (ResourceType type : resources.keySet()) {
- Map<String, Object> typedResources = resources.get(type);
- typedResources.forEach(this.resources.get(type)::putIfAbsent);
+ Map<String, Object> envTyped = resources.get(type);
+ if (envTyped == null || envTyped.isEmpty()) {
+ continue;
+ }
+ Map<String, Object> existing = this.resources.get(type);
+ // Rebuild to establish env-before-agent iteration order.
+ Map<String, Object> merged = new LinkedHashMap<>(envTyped);
+ merged.putAll(existing);
Review Comment:
`LinkedHashMap.putAll()` replaces values without moving existing keys. With
environment keys `[z, a]` and an agent override for `z`, the order becomes
`[agent-z, env-a]`, allowing environment skills to override agent skills. Could
we remove overlapping keys before appending agent entries? The
annotation-to-resource merge needs the same treatment.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -99,6 +106,22 @@ public void open() throws Exception {
this.prompt =
this.resourceContext.getResource((String) this.prompt,
ResourceType.PROMPT);
}
+
+ // Resolve skills_sources marker (plan-time source declarations) into
skill names,
+ // then merge with the descriptor's explicit "skills" list. Explicit
names come first;
+ // resolved names are appended, de-duplicated while preserving order.
+ List<SkillSourceSpec> skillsSources =
+ toSkillSourceSpecs(descriptor.getArgument("skills_sources"));
+ if (!skillsSources.isEmpty()) {
+ List<String> resolved =
this.resourceContext.resolveSkillNamesForSources(skillsSources);
+ Set<String> merged = new LinkedHashSet<>();
+ if (this.skills != null) {
+ merged.addAll(this.skills);
+ }
+ merged.addAll(resolved);
+ this.skills = new ArrayList<>(merged);
Review Comment:
Could we synchronize the resolved skill names into both cross-language
wrappers after the underlying model opens? `PythonChatModelSetup.open()` only
delegates to Python, leaving the Java wrapper’s `skills` at its original
descriptor value. With source-only configuration, `getSkills()` remains `null`,
so Java `ChatModelAction` omits the skill directories from the bash call’s
`allowed_script_dirs`, even after the bridge issue is fixed. The reverse
wrapper has the same gap: Python `JavaChatModelSetup.skills` is not refreshed
after Java `open()`. Copying the final list after each successful `open()`
should address this. Please verify the resulting bash arguments in both
directions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
