Skip to content

Tortoise#19

Merged
ludomitch merged 5 commits intomainfrom
tortoise
May 12, 2025
Merged

Tortoise#19
ludomitch merged 5 commits intomainfrom
tortoise

Conversation

@ludomitch
Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/scripts/deploy.py
Comment thread src/fhda/tortoise.py Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread src/fhda/tortoise.py Outdated
Comment thread .pre-commit-config.yaml
@ludomitch ludomitch marked this pull request as ready for review May 12, 2025 11:48
@ludomitch ludomitch merged commit b178f9d into main May 12, 2025
2 checks passed
@ludomitch ludomitch deleted the tortoise branch May 12, 2025 11:48
Comment thread src/fhda/tortoise.py
"success_rate": success_rate,
}

os.makedirs(f"{output_dir}/{step.step_id}", exist_ok=True)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note stuff like this won't work on Windows, as Windows paths are \\. Best to use os.path.join or pathlib joins

Comment thread src/fhda/tortoise.py
f"Here is the research question to address:\n"
f"<query>\n"
f"{query}\n"
f"</query>\n"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the trailing \n in the prompt? I think we should just have "</query>"

Comment thread src/fhda/tortoise.py
Comment on lines +17 to +19
class StepConfig(BaseModel):
"""Configuration for a step in the pipeline."""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What doesn't make sense to me is Step has extensive configuration as many Fields, then we also have a separate StepConfig. It would be good to either:

  • Rename StepConfig to be clearer
  • Consolidate StepConfig into Step

Comment thread src/fhda/tortoise.py
max_steps: int = Field(
default=30, description="Maximum number of steps for the agent"
)
timeout: int = Field(default=15 * 60, description="Timeout for the step in seconds")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because callers can have timeouts made by a formula that leads to a float.

In general, something like 2.5 seconds is a valid timeout

Comment thread src/fhda/tortoise.py
for prompt_text, prompt_args in prompt_pairs[
:task_count
]: # Limit to requested parallel count
step_copy = copy.deepcopy(step)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining why we deepcopy here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants