Skip to content

Conversation

@acured
Copy link
Contributor

@acured acured commented Oct 21, 2025

Issue:
Server part exit when all 3 tasks dequeued, but client part will hang there for queue out new task.
127.0.0.1 - - [21/Oct/2025 10:09:18] "POST /traces HTTP/1.1" 200 -

Not sure is it by design? but this example has not been fully executed, may confusing for beginners.

Fixed:
Make the number of tasks on the server and client consistent.

@acured acured force-pushed the hao/fix_apo_example_tasks_count_inconsistent branch from e27f784 to 812ee13 Compare October 22, 2025 00:43
@ultmaster
Copy link
Contributor

I think this inconsistency is intentional to let the readers know that the runners could run forever.

I agree that the setting max_tasks will make the flow more clear.

Please add a comment to let users know that this max_tasks is optional.

@ultmaster
Copy link
Contributor

We also need to find a way to trigger example workflows.

@ultmaster
Copy link
Contributor

Please merge from main and close-reopen to trigger ci-apo

@acured acured force-pushed the hao/fix_apo_example_tasks_count_inconsistent branch from 812ee13 to caefc0e Compare October 22, 2025 06:10
@ultmaster
Copy link
Contributor

May I know why you are still working on adding more features to legacy_*.py?


# 3. The algorithm waits for clients to process the task
rollout = await server.poll_completed_rollout(task_id, timeout=30)
rollout = await server.poll_completed_rollout(task_id, timeout=60)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add timeout and wait for the client to start.

@acured
Copy link
Contributor Author

acured commented Oct 22, 2025

I think this inconsistency is intentional to let the readers know that the runners could run forever.

I agree that the setting max_tasks will make the flow more clear.

Please add a comment to let users know that this max_tasks is optional.

Added

@acured
Copy link
Contributor Author

acured commented Oct 22, 2025

May I know why you are still working on adding more features to legacy_*.py?

Easier to trigger this example?

@acured acured closed this Oct 22, 2025
@acured acured reopened this Oct 22, 2025
@ultmaster
Copy link
Contributor

Please merge from main again and see if the tests work now.

I think it's a good opportunity to test CI for forked-origin PRs, because I usually open new branches on first-party repo.

@acured acured force-pushed the hao/fix_apo_example_tasks_count_inconsistent branch from c1be591 to ffb6d40 Compare October 24, 2025 08:32
@acured acured closed this Oct 24, 2025
@acured acured reopened this Oct 24, 2025
"""This is the APO example written in the legacy client-server style (agent-lightning v0.1).
New users should refer to the `examples/apo/apo.py` for the modern APO example.
New users should refer to the `examples/apo/legacy_apo_lunch.py` for the modern APO example.
Copy link
Contributor

Choose a reason for hiding this comment

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

legacy is not modern.

@@ -0,0 +1,87 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I highly doubt that this script will be useful:

  1. legacy_*.py is no longer maintained and used in future.
  2. CI used a bash to launch both script and no intermediate glue python script is used.

Also, it seems to me that this script might need a bit more love:

  1. lunch.py looks like a misspelling.
  2. The module docstring has bad formatting especially the last part. Suggesting letting copilot refine the wording.

"""This is the APO example written in the legacy client-server style (agent-lightning v0.1).
New users should refer to the `examples/apo/apo.py` for the modern APO example.
New users should refer to the `examples/apo/legacy_apo_lunch.py` for the modern APO example.
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants