refactor: remove gene_index step #946
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
documentation
DSuveges
approved these changes
Jan 22, 2025
| @@ -28,16 +27,16 @@ Available options: | |||
| Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace. | |||
| ``` | |||
|
|
|||
| As indicated, you can run a step by specifying the step's name with the `step` argument. For example, to run the `gene_index` step, you can run: | |||
| As indicated, you can run a step by specifying the step's name with the `step` argument. For example, to run the `gwas_catalog_sumstat_preprocess` step, you can run: | |||
There was a problem hiding this comment.
Thanks for updating the documentation! I keep forgetting about it.
Comment on lines
+572
to
+574
| def mock_target_index(spark: SparkSession) -> TargetIndex: | ||
| """Mock target index dataset.""" | ||
| ti_schema = TargetIndex.get_schema() |
There was a problem hiding this comment.
Theoretical question: wouldn't make more sense to use an actual data sample from the target index?
There was a problem hiding this comment.
Probably? There is a sample_target_index right above it in the code, but it doesn't seem to be used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✨ Context
Gentropy uses a
gene_indexthat is generated from the target index in the platform etl via thegene_indexstep. Thegene_indexstep is redundant, and we now want to use the target index from the platform etl directly.This PR is also connected to these PRs in the orchestration and platform-etl-backend repos.
Note: After this PR is merged, gentropy pipelines will depend on a
target_indexdataset generated by the platform etl that has atsscolumn and thus the gentropy pipeline cannot be run on its own. I have generated a patchedtarget_indexdataset usinggs://open-targets-pre-data-releases/24.12-uo_test-3/output/etl/parquet/targetspatched with thetsscolumn, just in case the gentropy pipelines need to be run on their own. The patchedtarget_indexcan be found here:gs://ot-team/vivien/gentropy_patched_datasets/target_index_with_tss_column🛠 What does this PR implement
gene_indexstep.gene_indexare renamed totarget_index.gene_indexessentially comprises of a subset of columns from thetarget_index, some field names differ, so the PR renames field names to be compatible with thetarget_indexwhere necessary.gene_indexjson schema has been replaced with thetarget_indexschema derived from atarget_indexdataset generated by the platform etl.gene_indexstep was used an example in the docs here and here. Thegwas_catalog_sumstat_preprocessstep is now used as the example as it has inputs of similar complexity.🙈 Missing
🚦 Before submitting
devbranch?make test)?poetry run pre-commit run --all-files)?