Skip to content

Asnyc testing #604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Aug 10, 2025
Merged

Asnyc testing #604

merged 22 commits into from
Aug 10, 2025

Conversation

ChrisPC-39
Copy link
Collaborator

@ChrisPC-39 ChrisPC-39 commented Jul 25, 2025

PR closes #254

Known issues:

  • aiomonitor-console doesn't load
  • snakeviz target (make profile-serve) appends the file path twice when opening a report (causing an error)

New targets

Make targets

  • make async-lint
  • make profile
  • make async-monitor
  • make async-debug
  • make profile-compare
  • make async-validate
  • make profile-serve
  • make async-benchmark
  • make async-clean

Github workflow

  • Async testing

Unit tests

  • tests/async/test_async_safety.py

@ChrisPC-39 ChrisPC-39 marked this pull request as ready for review August 4, 2025 11:27
@crivetimihai crivetimihai added this to the Release 0.6.0 milestone Aug 5, 2025
@crivetimihai
Copy link
Member

PR Review Comment - Action Items Required

🔄 Rebase Required

This branch is significantly behind main and needs rebasing before review.

✅ Steps to Fix:

1. Rebase on latest main

git fetch origin main
git rebase origin/main

2. Resolve conflicts - Keep these from main:

  • Version: 0.5.0 (not 0.4.0)
  • Dependencies: Latest versions from main
  • PyYAML dependency (don't remove)
  • Existing tool configurations

3. Preserve your async changes:

  • async_testing/ directory additions
  • ✅ MANIFEST.in entries for async_testing files -> why do we need to include this in the manifest?
  • ✅ pyrightconfig.json for async validation
  • ✅ Async-specific tool configs (if not duplicating)
  • ✅ New dependencies: aiohttp, websockets -> not sure why we need these depends, are these dev only?

4. Verify after rebase:

  • Version is 0.5.0
  • No dependency downgrades
  • PyYAML still present
  • Tests pass: pytest tests/async/
  • Linting passes: make lint
  • Type checking passes: pyright

5. Clean up commits:

  • Consider squashing related commits
  • Ensure commit messages are clear about async testing additions

⚠️ Current Issues:

  • Version regression (0.5.0 → 0.4.0)
  • Multiple dependency downgrades
  • Missing PyYAML dependency
  • Mixed concerns in commits

Once rebased, the async testing additions look good and can be properly reviewed.

@ChrisPC-39
Copy link
Collaborator Author

ChrisPC-39 commented Aug 8, 2025

Q&A

MANIFEST.in entries for async_testing files -> why do we need to include this in the manifest?

Github workflows doesn't recognize the new scripts if not included in Manifest

New dependencies: aiohttp, websockets -> not sure why we need these depends, are these dev only?

Moved to dev dependencies as they are indeed dev only. Oops!

Checks

  • Rebased to latest main branch
  • Version is 0.5.0
  • No dependency downgrades -> rebased and ensured dependency versions
  • PyYAML still present
  • Tests pass: pytest tests/async/
  • Tests pass: make test
  • Linting passes: make lint -> Due to large number of file changes in make lint (41 files changed, 263 insertions(+), 341 deletions(-)), I will make a separate PR for linting.
  • Type checking passes: pyright -> make pyright fails with 1418 errors. Due to the large number of errors, will solve them in an upcoming PR (errors are not due to my changes)

Sebastian and others added 22 commits August 10, 2025 14:59
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Sebastian <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
@crivetimihai crivetimihai self-assigned this Aug 10, 2025
@crivetimihai
Copy link
Member

Updated PR - Rebased onto main and fixed formatting/linting issues:

  1. Rebased onto main - Successfully rebased the asnyc_testing branch onto the latest main branch (35 commits) without conflicts
  2. Fixed isort/black formatting cycle - Resolved persistent reformatting issues by restoring proper isort configuration:
  • Added missing [tool.isort] header
  • Restored critical settings: multi_line_output = 3, include_trailing_comma = true, from_first = true
  • These settings ensure isort and black agree on import formatting
  1. Fixed pylint warnings - Achieved 10.00/10 score:
  • Changed FIXME comments to TODO with # pylint: disable=fixme for legitimate placeholders in mcpgateway/handlers/sampling.py
  1. Fixed MANIFEST.in - Resolved check-manifest verification:
  • Changed prune llms-full.txt to exclude llms-full.txt (prune is for directories, exclude for files)

All CI checks should now pass cleanly. The branch is up-to-date with main and all formatting/linting tools are satisfied.

Copy link
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

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

Rebased and fixed all issues

@crivetimihai crivetimihai merged commit 1d4ba08 into IBM:main Aug 10, 2025
36 checks passed
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.

[CHORE]: Async Code Testing and Performance Profiling Makefile targets (flake8-async, cprofile, snakeviz, aiomonitor)
2 participants