feat: DIA-1384: add cost estimate endpoint #225
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
==========================================
- Coverage 66.41% 65.59% -0.82%
==========================================
Files 46 47 +1
Lines 2230 2343 +113
==========================================
+ Hits 1481 1537 +56
- Misses 749 806 +57 ☔ View full report in Codecov by Sentry. |
adala/agents/base.py
Outdated
| @@ -216,6 +216,16 @@ def get_teacher_runtime(self, runtime: Optional[str] = None) -> Runtime: | |||
| ) | |||
| return runtime | |||
|
|
|||
| def get_skills(self) -> List[Skill]: | |||
There was a problem hiding this comment.
why do we need this function? Skills must be always Dict mapping from skill name to Skill (there is a skills_validator function to prevalidate that), so we can always iterate like
for skill_name in agent.skills.get_skill_names():
skill = agent.skills[skill_name]
There was a problem hiding this comment.
The type sig for skills is skills: SerializeAsAny[Union[Skill, SkillSet]], so we can't count on this always being a SkillSet (and therefore having get_skill_names defined), yeah?
Plus, it seems simpler to be able to get the list of skills than the list of names, I'd think? Since it doesn't seem that get_skill_names is being used, I think it's simpler to change that to get the skills themselves (if we can, in fact, assume this is a SkillSet)
There was a problem hiding this comment.
so we can't count on this always being a SkillSet
we can because of https://github.com/HumanSignal/Adala/blob/master/adala/agents/base.py#L109
There was a problem hiding this comment.
Can we change the type sig of skills in Agent then?
And still, like I said in my first comment, it seems simpler to have a SkillSet.get_skills than SkillSet.get_skill_names, esp since SkillSet.get_skill_names doesn't seem to be used anywhere. I'll go ahead and change that and the type sig of Agent.skills
No description provided.