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

Cleanup task-collection modules and tests #1190

Closed
tcompa opened this issue Jan 25, 2024 · 1 comment · Fixed by #1188
Closed

Cleanup task-collection modules and tests #1190

tcompa opened this issue Jan 25, 2024 · 1 comment · Fixed by #1188

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jan 25, 2024

The tasks/collection.py module is quite messy, and its tests are scattered in different modules with no obvious logic. This is confusing, while developing e.g. PR #1188.

Within tasks, we should separate e.g.:

  1. Schemas or any payload validation logic;
  2. Functions that are used within the task-collection endpoint (including preliminary tests - ref Prevent task-package name conflict #1189);
  3. Functions that are executed as a background task.

A similar organization can be used for unit/API tests.

This will help us to clearly determine which functions rely on which assumptions (e.g. which functions use normalized package names).

@tcompa
Copy link
Collaborator Author

tcompa commented Jan 26, 2024

I will close this with #1188, which includes a refactor that does not change functionality (on top of the actual changes related to #736). The new structure is as follows

$ tree fractal_server/tasks/
fractal_server/tasks/
├── background_operations.py
├── endpoint_operations.py
├── __init__.py
├── _TaskCollectPip.py
└── utils.py

0 directories, 5 files

$ tree tests/07_task_collection/
tests/07_task_collection/
├── test_background_operations.py
├── test_endpoint_operations.py
└── test_unit_tasks.py

0 directories, 3 files

which at least places definitions in some appropriately scoped module.

Note that a review of the task-collection flow is still due, although not urgent. As an example, to start with, the fact that we often call the check method is a hint that some abstractions are not correct.

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

Successfully merging a pull request may close this issue.

1 participant