Repository navigation
Conversation
project-defiant
left a comment
There was a problem hiding this comment.
Impressive work, if we are going to make the variant lighter (which would make sense due to the fact that it mixes the transcript as well) probably it's worth to get rid of pyspark.
There was a problem hiding this comment.
Probably worth joining with existing solution for evidence.
There was a problem hiding this comment.
Oh, I made the same comment, I haven't seen yours.
| def _rescale(score: pl.Expr, low: float, high: float, minimum: float, maximum: float) -> pl.Expr: | ||
| # Spark subtracts an integer bound in single precision and a fractional one in double. | ||
| shifted = score - low if isinstance(low, int) else score.cast(pl.Float64) - low | ||
| return divide(shifted, high - low) * (maximum - minimum) + minimum |
There was a problem hiding this comment.
I think this also lives for min_max_scaler for foldx or lof, worth to check if can be reused
There was a problem hiding this comment.
Done in 348054a. _rescale existed only to reproduce Spark's mixed single/double precision, and that isn't worth a second scaler. AlphaMissense, CADD and GERP normalisedScore values may move in the last digits against the previous release.
There was a problem hiding this comment.
Now that the step is run on a single machine, I would get rid of foldx step and embed it here. No need to spawn machine two times.
There was a problem hiding this comment.
Agreed, done in c6168f6. FoldX is now the first task of the variant step, and pts_foldx is removed from the DAG. Only the variant index reads its output.
| ) | ||
|
|
||
|
|
||
| def hash_variant_index_ids( |
There was a problem hiding this comment.
Shouldn't this method be used in other resources where we need variant hashing? eg. EVA/ClinVar?
There was a problem hiding this comment.
I also not sure if we should have transformers/utils/variants.py when we have transformers/variant_utils.
There was a problem hiding this comment.
Ironically, the two hashing functions were behaving differently when the variant had exactly 300 characters 🙃. This is another benefit of converging codebases
There was a problem hiding this comment.
Dang. I did a lot of checks when the duplicated function was added to gentropy and the etl, but I might have missed the equality. Also, @SzymonSzyszkowski pointed out that when the evidence logic was migrated to polars, the hashing function produces different output. In relation to generating evidence ids, I knew this,but forgot the implication this discrepancy caused in the variant index generation.
There was a problem hiding this comment.
On the naming: good point. The step is now one transformers/variant/ package (f2ccc4b), laid out like transformers/target/. transformers/utils/variant.py stays for the helpers that evidence, LoF curation and FoldX share with it.
annotation_parts_by_chromosome scanned every part with the first part's schema, so a later part with a column the first lacked raised a SchemaError before any annotation was read. It now scans only `chromosome`, ignoring other columns, as read_variant_annotations already does for the same parts.
… keys without FoldX list_parts in utils/dataset.py now holds the directory-or-file part listing that scan_dataset, list_vep_parts and annotation_parts_by_chromosome each repeated. The variant step collects the amino-acid keys of the parsed index only when an amino-acid annotation source is configured.
variant.py and its private variant_utils/ helpers move into one transformers/variant/ package, as transformers/target/ is laid out, so the step's own modules are no longer confused with the shared transformers/utils/variant.py. The `variant` transformer still resolves from pts.transformers.variant.
normalise_score drops its own _rescale, which reproduced Spark's mixed single and double precision arithmetic, and rescales with min_max_scaler in double precision. AlphaMissense, CADD and GERP normalisedScore values can move in the last digits compared with the Spark output.
The FoldX transform is the variant step's first task, required by the variant index, in both the platform config and the PPP override, so the two no longer start a VM each. pts_foldx leaves the DAG and pts_variant depends on pis_variant directly; FoldX output is read only by the variant index.
…nd the variant index hash_variant_index_ids and hash_long_variant_ids disagreed on IDs of exactly 300 characters: evidence and LoF curation hashed only longer IDs, the variant index hashed at the threshold, so an evidence variantId of exactly 300 characters never joined the index. The two merge into hash_long_variant_ids with the variant index's rule. Coordinates come from columns when the caller has them, as LoF curation and the variant index do, and are otherwise extracted from the ID as evidence did. The only change to evidence output is that 300-character IDs are now hashed.
This ports the variant index step from #205 to polars, replacing the Spark variant step on
main.@project-defiant, as the author of the Gentropy migration in #205, your view on this would be very useful.
Where I stand
The variant step sits on the boundary between the two worlds, and both implementations have real pros and cons. I now lean towards polars. It needs about half the compute, runs on the default PTS VM, and leaves Dataproc to Gentropy alone. Since review it also shares its hashing, scaling and part-listing helpers with the rest of PTS, and FoldX runs in the same step. The Spark version is still less code, and it would scale further if the variant index grew by an order of magnitude. Both sides are laid out below.
Spark vs polars
Both versions ran on the same inputs (
sz/ppp-2610-1), and their outputs were compared value by value, one chromosome at a time.consequences.pyand schema)Parity: the outputs match.
7_142349257_G_C, two FoldX predictions tie and each implementation keeps a different one. Neither choice is more correct than the other.normalisedScorevalues are now rescaled in double precision with the sharedmin_max_scaler, so they can differ from Spark in the last digits.mainon purpose. VEP writes OMIM IDs as JSON numbers and sometimes drops the trailing zero of the four-digit variant suffix. In a sample of 50 VEP parts, 17 of 401 IDs looked like159530.001for159530.0010.mainreads the raw text, so it writes159530#001for these.159530#0010, the real OMIM ID.Arguments for polars
pts_variantDataproc profile, the init action and the Spark settings the step depended on (ANSI off, adaptive query execution, zstd codec). If this goes ahead, Dataproc stays only for Gentropy.collect_setand changing type coercion under ANSI.schemas/vep.pyplus one expression.config.yaml, provided it followsvariant_schemaor the FoldX shape.pts_evidence_europepmcalready uses that override.Arguments for PySpark
Less, more declarative code. The polars version adds orchestration that Spark does for us: a loop over chromosomes, batching, a process pool, a scratch directory, and a hand-written union that touches only the rows that need it. That is about 20% more code, all of it specific to this step.
Horizontal scaling. Polars can only grow by moving to a bigger VM.
Nested data is the weak spot of polars 2. The variant index is mostly
list[struct]columns, and the port hit several polars traps along the way:sort_byinsidelist.evalfails on empty lists;list.arg_maxreturns the last maximum for strings but the first for floats;when/thenpanics on multi-chunk frames;replace_strictinsidelist.evalis rebuilt for every row;eq_missingon nested lists is very slow.The tests now cover each of these, but new nested logic is likely to hit more. Spark's
array_unionandsort_arrayare mature.Gentropy lineage. The step comes from Gentropy's
variant_index. If we expect to keep pulling changes from Gentropy, the Spark version can take them almost line for line, while the polars version needs each one re-ported.Changes since review
list_partshelper inutils/dataset.pyreplaces three copies of the part-listing logic, and the FoldX lookup keys are only computed when a FoldX source is configured.transformers/variant/package, laid out liketransformers/target/.min_max_scalerinstead of a Spark-precision_rescale.variantstep, andpts_foldxleaves the DAG.What this PR contains
Step
pts/src/pts/transformers/variant/__init__.pyis a newvarianttransformer that replaces thepyspark/variant.pyjob.spawnprocesses, one per CPU by default, each with one polars thread. Each part is written to its own local Parquet file.hash_threshold;parse_processes(one per CPU by default) andparse_threads(polars threads per parsing process, default 1);batch_size.pts/src/pts/transformers/variant/vep.pyparses VEP records:pts/src/pts/transformers/variant/index.py:min_max_scaler;array_unionsemantics and its descending sort of struct arrays;pts/src/pts/schemas/vep.pyholds a VEP schema pruned to the fields the step reads, so thatscan_ndjsonskips everything else.pts/src/pts/transformers/utils/variant.pyhas onehash_long_variant_idsfor evidence, LoF curation and the variant index.variantIdcould never join the index.pts/src/pts/transformers/utils/dataset.pygainslist_parts, whichscan_datasetand the variant step share to list a dataset's parts.variant/consequences.pymoves frompyspark/variant_utils/unchanged.Config and orchestration
pts/config.yamlandppp/pts.override.yamlbecomestransform variant indexwithtransformer: variant, andoutput_partitionsgoes. Without apyspark …task,pts_variantruns on a GCE VM.variantstep.transform FoldX annotationsis the step's first task andtransform variant indexrequires it, in bothpts/config.yamland the PPP override.pts_foldxis removed fromunified_pipeline.yaml, andpts_variantnow depends onpis_variantandgentropy_variant_annotation.pts_variantDataproc profile is removed fromclusters.yaml.pts_variantto run on GCE, with FoldX inside it.Removed
pts/src/pts/pyspark/variant.pypts/src/pts/pyspark/variant_utils/(spark.py,index.py,vep.py)pts/tests/test_pyspark_variant.pyTests
pts/tests/test_variant.pyports all of the Spark tests.How it was verified
sz/ppp-2610-1, compared value by value with the Spark output of feat(pis,pts): generate variant annotations and the variant index within the pipeline #205 for the same run. The results are above.🤖 Generated with Claude Code
https://claude.ai/code/session_015KGXafVcd7q9w8y1uKHc7u