-
Notifications
You must be signed in to change notification settings - Fork 22
feat: move clickhouse load to luigi #1186
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
Conversation
…es into benb/move_clickhouse_load_to_luigi
…es into benb/move_clickhouse_load_to_luigi
| run_id=run_id, | ||
| **lpr.model_dump(exclude='request_type'), | ||
| ) | ||
| if FeatureFlag.CLICKHOUSE_LOADER_DISABLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope for this change for sure, but is there a reason to continue to support this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to support the loader running for open source users until we are sure everyone has migrated from hail->clickhouse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The seqr helm charts we are releasing are totally non-functional if you have not migrated and have been for weeks. In our release notes we told people to use the 1.45.0-hail-search-final chart if they have not yet finished the migration, but if you update to the v2 charts the assumption is that you can not continue to have a working hail search running. I do not think we should continue to support this feature in the v2 charts indefinitely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, 1.45.0-hail-search-final does not actually include the necessary steps to actually run the migration. We added the steps and documentation in the 2.0.0 release. The 2.x.x charts don't support hail-search but they do need to fully support the mechanism to export the hail tables over.
I just realized I can actually add similar code to the migration code to what I did here, which would actually allow deprecation of the loader. I'll go do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah that makes sense
this is unblocked now.