Skip to content

Conversation

@booxter
Copy link
Contributor

@booxter booxter commented Apr 15, 2025

No description provided.

@mergify mergify bot added the ci-failure label Apr 15, 2025
@JamesKunstle
Copy link
Contributor

Why do we need the asyncio stuff in this lib anyway? I've been wondering if we can just drop that requirement altogether

@booxter
Copy link
Contributor Author

booxter commented Apr 15, 2025

We can definitely drop it at least until we feel the need. I will drop.

@booxter booxter force-pushed the asyncio_default_fixture_loop_scope branch from e655eb2 to 1d7ae1c Compare April 15, 2025 22:22
Signed-off-by: Ihar Hrachyshka <[email protected]>
@mergify mergify bot added CI/CD Affects CI/CD configuration testing Relates to testing labels Apr 15, 2025
@mergify
Copy link
Contributor

mergify bot commented Apr 15, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. @booxter please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Apr 15, 2025
@booxter booxter force-pushed the asyncio_default_fixture_loop_scope branch from 1d7ae1c to 48a5791 Compare April 15, 2025 22:23
@booxter booxter changed the title chore: Set asyncio_default_fixture_loop_scope for pytest chore: Drop pytest-asyncio Apr 15, 2025
@mergify mergify bot added the ci-failure label Apr 15, 2025
@RobotSail
Copy link
Member

@JamesKunstle We use async logic in order to optimize I/O logic and optimize the amount of CPU time available so the training loop doesn't hang on blocking FS write calls. You can see this in action if you look at the AsyncStructuredLogger implementation here: https://github.com/instructlab/training/blob/main/src/instructlab/training/async_logger.py

Copy link
Member

@RobotSail RobotSail left a comment

Choose a reason for hiding this comment

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

We can remove this now, but I have a feeling we will need to just bring it back in again later.

@mergify mergify bot added the one-approval label Apr 16, 2025
@booxter booxter requested a review from JamesKunstle April 16, 2025 16:52
Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

Choose a reason for hiding this comment

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

We can re-add this when we have tests that need it. For now it's tidier to just remove it.

@JamesKunstle JamesKunstle merged commit fdab523 into instructlab:main Apr 16, 2025
12 of 13 checks passed
@mergify mergify bot removed the one-approval label Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Affects CI/CD configuration ci-failure testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants