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

/history/by_task_id : a method to link each task's history uniquely to the SQL database #3321

Merged
merged 15 commits into from
Nov 12, 2024

Conversation

tashrifbillah
Copy link
Contributor

I am a visionary that has been utilizing Luigi for medical research since 2020. Our users are non-programmers who need convenience over complexity to examine a task's history. I scrutinized your db_task_history.py and server.py and figured out how it could made easy for non-programmers. My proposed method has been working since 2020. To complete this PR, I have enriched the documentation and added screenshot.

Description

No change in the existing functionality. Only added /history/by_task_id/ endpoint.

Motivation and Context

The existing method for getting a single task's history in future is not adequate. To get so, one has to remember the numeric id of a task. This PR omits that burden by providing a method to autonomously obtain a single task's history. As a result, we can now construct history trees and link each task's history uniquely to the SQL database.

Have you tested this? If so, how?

I ran my jobs with this code and it works for me.

@tashrifbillah tashrifbillah requested review from dlstadther and a team as code owners November 8, 2024 01:26
@tashrifbillah
Copy link
Contributor Author

Hi @PeteW , I hope you are well. We interacted last in 2020. This is the remnant of what came out of that interaction. Being inspired by the tree functionality, I provided a way to autonomously obtain a single task's history in order to construct an HTML tree. Can you please review it?

@PeteW
Copy link
Contributor

PeteW commented Nov 8, 2024

Good to hear from you! Yes I remember. I'll have a response for you by tomorrow

@PeteW
Copy link
Contributor

PeteW commented Nov 9, 2024

This is great. The coverage tests indicate a warning about adding small methods without coverage. However from my standpoint this is a nitpick. The change makes sense and thanks for bringing me back to review this great project.

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Please look at the failing flake8 test.

Also @RRap0so , would you or another admin of the repo be able to look into why PRs keep expecting results from the base (3.9, docs) and base (3.9, flake8) tests (these were replaced by newer tests). Based on a quick search, it sounds like there might be branch protection rules specified on master which requires series of specific test successes. I don't have access to the luigi Settings page to check for myself.

luigi/db_task_history.py Outdated Show resolved Hide resolved
@dlstadther dlstadther merged commit 172128c into spotify:master Nov 12, 2024
48 checks passed
@tashrifbillah
Copy link
Contributor Author

Thank you all. Just installed the master branch and verified that all functionalities are intact!

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.

4 participants