VIRify followup to ASA pipeline #61
Conversation
SandyRogers
changed the title
* fix dev container build/deps * prefect+pyslurm upgrade fixes * updates for py3.12 / prefect 3.4
Co-authored-by: Sandy Rogers <[email protected]>
Co-authored-by: Varsha Kale <[email protected]>
Often library_strategy metadata is wrong/missing in ENA. These policy allow prefect user to select an option for including/overriding/trusting the provided metadata.
Otherwise, potentially AMPLICON runs are fetched from ENA, but not included in samplesheets.
Also added a CLAUDE.md file to help the robots.
mberacochea
force-pushed
the
feature/asa-virify
branch
from
e5f4ec4 to
c34107b
Compare
mberacochea
force-pushed
the
feature/asa-virify
branch
from
a05efe9 to
54489bf
Compare
There was a problem hiding this comment.
@mberacochea thanks for tackling the big schema question. I like the compositional schema approach a lot, and also the attempt at brining DownloadFile and the file-rules' File node closer. I think we should bring them completely together, and I left some other ideas scattered through about it too. Happy to talk in person if easier - ta!
| elif isinstance(file_item, tuple) and len(file_item) == 2: | ||
| # If it's a tuple of (filename, rules), create a File with those rules | ||
| filename, file_rules = file_item | ||
| directory.add_file(filename, rules=file_rules) | ||
| elif isinstance(file_item, str): | ||
| # If it's a string, create a File with default rules | ||
| directory.add_file(file_item) | ||
| else: | ||
| raise ValueError(f"Unsupported file specification: {file_item}") |
There was a problem hiding this comment.
Neat, I like the default handling if passing strings.
However:
- a
pathlib.Pathobject is as likely as astrso it probably makes sense to handle that the same? - I've probably had too much pydantic koolaid, but similar to the
create_filefactory, I would argue thatDirectorys should be created directly rather than wrapped like this. In pydantic these conditions can be handled with pre validators. Doing so also means the ValueError would happen automatically.
(Unless I am missing something again obviously)
There was a problem hiding this comment.
Agreed, I've deleted the factory method
|
|
||
| if not allow_non_exist: | ||
| glob_rules.append(GlobOfTaxonomyFolderHasHtmlAndKronaTxtRule) | ||
| class AssemblyDirectorySchema(BaseModel): |
There was a problem hiding this comment.
Nice. I guess adding explicit subdirectories support to the filerules' node Directory would cover this right?
| except FileExistsError: | ||
| logger.warning( | ||
| f"Download with alias {download.alias} already exists for analysis {analysis.accession}" | ||
| ) |
There was a problem hiding this comment.
should this only be a warning?
workflows/flows/analyse_study_tasks/run_virify_pipeline_via_samplesheet.py
Show resolved
Hide resolved
workflows/flows/analyse_study_tasks/run_virify_pipeline_via_samplesheet.py
Show resolved
Hide resolved
| @@ -0,0 +1,358 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
file naming: why ztest? this probably breaks pytest discovery
There was a problem hiding this comment.
yeah, I've disabled that one as it's currently broken
| @@ -0,0 +1,282 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Same q: why ztest in file name?
This PR:
There is a bit of work to do: