Skip to content
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

Refactor and cleanup #7

Merged
merged 7 commits into from
Mar 11, 2025
Merged

Refactor and cleanup #7

merged 7 commits into from
Mar 11, 2025

Conversation

andrejridzik
Copy link
Collaborator

@andrejridzik andrejridzik commented Feb 28, 2025

Change

Slight refactoring and cleanup of the code.

This change includes addition of Ruff formatter and creation of Github workflows.

How to Test

Does not require any additional testing as functionality is not being changed

Checklist

  • Tests have been added or updated to reflect the changes, or their absence is explicitly explained.
  • Documentation has been added or updated to reflect the changes, or their absence is explicitly explained.
  • A self-review has been conducted checking:
    • No unintended changes have been committed.
    • The changes in isolation seem reasonable.
    • Anything that may be odd or unintuitive is provided with a GitHub comment explaining it (but consider if this should not be a code comment or in the documentation instead).
  • All CI checks pass before pinging a reviewer, or provide an explanation if they do not.

Related Issues

None

@andrejridzik andrejridzik self-assigned this Feb 28, 2025
@andrejridzik andrejridzik force-pushed the feature/refactor-and-cleanup branch from 2b5e981 to 3687951 Compare February 28, 2025 12:58
@andrejridzik andrejridzik force-pushed the feature/refactor-and-cleanup branch from 3687951 to 5a3663a Compare February 28, 2025 13:00
@andrejridzik andrejridzik force-pushed the feature/refactor-and-cleanup branch from d8bcb0b to 49d0277 Compare March 7, 2025 08:43
@andrejridzik andrejridzik changed the title Draft: Refactor and cleanup Refactor and cleanup Mar 7, 2025
Copy link
Collaborator

@marcel-vesely-kinit marcel-vesely-kinit left a comment

Choose a reason for hiding this comment

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

The only thing I'm unsure of is the use of asyncio.run that I've commented for you.

Both versions, with or without the asyncio.run wrapper did work correctly. Actually it seems like that the current contents of the start_async_thread function might be doing a very similar job to the asyncio.run function.

@andrejridzik andrejridzik force-pushed the feature/refactor-and-cleanup branch from 5689aaa to 8f69f7b Compare March 11, 2025 11:12
@andrejridzik andrejridzik force-pushed the feature/refactor-and-cleanup branch from 8f69f7b to 8c2c875 Compare March 11, 2025 11:13
Copy link
Collaborator

@marcel-vesely-kinit marcel-vesely-kinit left a comment

Choose a reason for hiding this comment

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

I've actually encountered a warning regarding the proper cleanup of resources, but it's not related to the way how we start off the async function in a new thread (or at the very least, I get the same warning regardless of whether I use the old or the new implementation of functions found in the threads.py file)

Warning: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown

How to reproduce (if you're interested): Start the app from terminal using uvicorn command and then, once everything is set up, terminate the app by using CTRL C shortcut.

TODO: For now I shall put it to backlog, it's not that pressing at the moment. The number of leaked semaphores doesn't increase with the number of spawned new threads utilizing a scheduler which is nice...

@andrejridzik
Copy link
Collaborator Author

I've actually encountered a warning regarding the proper cleanup of resources, but it's not related to the way how we start off the async function in a new thread (or at the very least, I get the same warning regardless of whether I use the old or the new implementation of functions found in the threads.py file)

Warning: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown

How to reproduce (if you're interested): Start the app from terminal using uvicorn command and then, once everything is set up, terminate the app by using CTRL C shortcut.

TODO: For now I shall put it to backlog, it's not that pressing at the moment. The number of leaked semaphores doesn't increase with the number of spawned new threads utilizing a scheduler which is nice...

Adding a link to the newly created issue #24 that captures this.

@andrejridzik andrejridzik merged commit 06a7d67 into main Mar 11, 2025
1 check passed
@andrejridzik andrejridzik deleted the feature/refactor-and-cleanup branch March 11, 2025 13:44
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