-
Notifications
You must be signed in to change notification settings - Fork 0
feat: CLIN-4328 - batch ids and sequencing ids Part 1 #584
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
Je présume que la partie dans etl.py dédiée à enrich sera couverte plus tard? |
dags/lib/utils.py
Outdated
if type(lst) is str: | ||
lst = [lst] | ||
if lst and len(lst) > 0 and all(s != "" for s in lst): |
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.
Ici, si l'un des éléments de la list est un string vide, on renvoie une liste vide. C'est bien ce qu'on veut et pas, par exemple, juste enlever les string vide de la liste?
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.
je vais supprimer cette fonction
dags/lib/utils.py
Outdated
if type(lst) is str and lst.strip() != "": | ||
lst = [lst] | ||
if lst and len(lst) > 0 and all(s.strip() != "" for s in lst): |
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.
Je vois que le petit comportement qui m'inquiétait est encore présent. Ici, quand un des éléments de la liste est un string vide, on retourne une liste vide. On ne voudrait pas juste filtrer ces éléments au lieu de retourner une liste vide?
dags/lib/utils_etl.py
Outdated
if sequencing_ids: | ||
arguments = arguments + ['--sequencing_ids', sequencing_ids] # probably wont work out of the box, cause sequencing_ids is a XCom param |
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.
Si on met un type liste dans l'ETL, il faut passer --sequencingId pour chaque sequencing_id à passer. Aussi je suggère de respecter le case de batchId (camelCase).
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.
Et si la liste de sequencing_ids provient du output de la task get_sequencing_ids, tu auras bien une liste et non un Xcom.
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.
airflow est pas de ton avis sur ce dernier point ... peut-etre je deplacerai build_etl_job_arguments
dans la partie execute
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 bah parfait ca a fonctionne, donc je confirme que build_etl_job_arguments a cet endroit du code batch_id et sequencing_ids sont toujours sous forme de XCOM, param templates.
Je l'ai change de place et mis dans execute() et la c'est tout beau
Arguments for Spark job: ['snv_somatic', '--config', 'config/qa.conf', '--steps', 'default', '--app-name', 'etl_normalize_snv_somatic', '--sequencingId', '1265158,1265143']
...
Exception in thread "main" java.lang.Exception: Missing argument: -b --batchId <str>
Unknown arguments: "--sequencingId" "1265158,1265143"
Expected Signature: snv_somatic
-c --config <str> Config path
-s --steps <steps> Steps
-a --app-name <str> App name
-b --batchId <str> Batch id
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.
"--sequencingId" "1265158,1265143"
faudrait que ce soit "--sequencingId" "1265158" "--sequencingId" "1265143"
pour que mainargs le traite comme une liste.
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.
Je donne le approve pour clarifier que mes commentaires ont été adressés.
Cependant, je crois que c'est pertinent de régler le problème avec la logique dupliquée avec Franklin et bien nous entendre sur quand on veut utiliser les sequencing ids et quand on veut utiliser les analysis id.
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.
J'utiliserai tes versions de fonctions dans Franklin sans problème. Juste checker pour passer la liste de --sequencingId à mainargs.
PR qui rajoute la possibilitee de passer
sequencing_ids
en param de:etl
etl_ingest
Cela s'inscrit dans la futur flot ou
etl_run
, une fois la generation des VCFs, Exomisers termine, pourra appeleretl
avec la liste des sequening_ids et realiser une release comme on a l'habitude de faire de facon transparente.La scope de la PR a ete volontairement reduit et n'inclut pas la partie ou les ids sont propages dans le code et envoyes vers les ETLs, ca faisait trop gros.
Le but de la PR est aussi de rester retro-compatible avec l'existant, tout les endroit qui utilisent
detect_batch_type
pour savoir ce qui doit etre skip ou pas.Voici a quoi ressemble
etl
:Vous avez ici un exemple ou un ensemble de batchs ainsi que de sequencings ont ete envoyes en param,
etl
accepte ce cas maisetl_ingest
va se plaindre si on lui demande de se lancer sur des choses qui ont des analyses type differents.etl
va donc realiser un routage:etl_ingest
par batch_idetl_ingest
soit 1 ou 2 fois.task_instance.xcom_pull(task_ids='detect_batch_type')
devraient toujours foncitonner pour determiner ce qui doit etre fait ou skip.Ainsi
etl_ingest
se retrouve toujours dans une situation ou une run = 1x batch type ce qui permet de garder toute la logique existante.detect_batch_type
a eut bcp de modifs et d'aller-retours d'idees, dont on peut discuter.