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

feat: distance-based task distribution logic #119

Merged
merged 13 commits into from
Jan 26, 2023
Merged

feat: distance-based task distribution logic #119

merged 13 commits into from
Jan 26, 2023

Conversation

Ayush5120
Copy link
Contributor

Added task distribution logic, that computes the total distance between the set of input URIs (from the incoming TES task) and the available TES instances, then relays the task to the closest TES instance.

@Ayush5120 Ayush5120 self-assigned this Jan 20, 2023
@uniqueg uniqueg changed the title Improvised task distribution logic feat: distance-based task distribution logic Jan 22, 2023
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Thanks @Ayush5120. There are quite some comments I made. Please address those and let's have another round of review after.

pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Show resolved Hide resolved
pro_tes/middleware/task_distribution.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Show resolved Hide resolved
pro_tes/middleware/task_distribution.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware.py Show resolved Hide resolved
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Great - we're getting there! :)
I think all the remaining issues are small fry that shouldn't take long to address

pro_tes/middleware/__init__.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/models.py Outdated Show resolved Hide resolved
pro_tes/middleware/task_distribution/__init__.py Outdated Show resolved Hide resolved
pro_tes/middleware/task_distribution/distance.py Outdated Show resolved Hide resolved
pro_tes/middleware/task_distribution/distance.py Outdated Show resolved Hide resolved
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Ayush5120. I didn't try it out, really, but it looks good to me and I will merge it. I hope edge cases (e.g., no inputs at all) will work. But if not, we can fix them after merging. Note that I have opened #124 to handle middleware more gracefully and generically. It's not an urgent issue, though - we can take care of it after #123 and #122.

pro_tes/middleware/middleware.py Show resolved Hide resolved
@uniqueg
Copy link
Member

uniqueg commented Jan 26, 2023

Hi @Ayush5120. Thanks a lot for the great work :)

I am merging now, as I guess you have addressed at least most of the comments (and I have opened #124). If there are any more issues popping up during testing, we can open individual issues for them and fix them :)

In the future, I think it's best if you resolve open conversations yourself if you feel confident that you have addressed them non-controversially. Otherwise, please leave a comment, either describing how you addressed it, or why you did not address it. That makes reviewing a whole lot easier.

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.

None yet

2 participants