Skip to content

Crispr paml#116

Merged
golharam merged 15 commits into
mainfrom
crispr-paml
Jun 30, 2026
Merged

Crispr paml#116
golharam merged 15 commits into
mainfrom
crispr-paml

Conversation

@yfan2012

@yfan2012 yfan2012 commented Jun 12, 2026

Copy link
Copy Markdown

Summary

New abstract methods added to Platform base class

  • copy_file() — Copy a single file between projects
  • get_current_user() — Return authenticated user profile as a standardized dict

_find_or_create_path() — SevenBridges

Fix: Root path '/' now returns None instead of trying to create a folder with an empty-string name. Changed from stripping the leading empty string to filtering out all empty strings from the split.

Unit tests (test_sevenbridges_platform.py):

  • test_find_or_create_path_root_returns_none

Integration tests (test_find_or_create_path.py):

  • test_root_path_returns_none
  • test_creates_single_folder
  • test_creates_nested_path
  • test_existing_folder_is_reused
  • test_partially_existing_path

upload_file() — SevenBridges

Fix: dest_folder='/' now correctly uploads to root. After stripping the trailing slash, an empty string skips _find_or_create_path and sets parent_folder_id = None. Also safely handles _find_or_create_path returning None.

Unit tests (test_sevenbridges_platform.py):

  • test_upload_file_to_rootdest_folder=None → project root
  • test_upload_file_to_subfolder — creates path, uploads into it
  • test_upload_file_with_destination_filename — renames on upload
  • test_upload_file_skips_existing_when_no_overwrite — returns existing id
  • test_upload_file_overwrites_existing — uploads despite existing file
  • test_upload_file_returns_none_on_failure — failed status → None
  • test_upload_file_strips_trailing_slash/inputs//inputs
  • test_upload_file_dest_folder_just_slash'/' → root
  • test_upload_file_extracts_basename/home/user/file.txtfile.txt

Integration tests (test_upload_file_integration.py):

  • test_upload_to_root
  • test_upload_to_subfolder
  • test_upload_to_nested_subfolder
  • test_upload_with_destination_filename
  • test_upload_no_overwrite_returns_existing_id
  • test_upload_with_overwrite
  • test_upload_to_root_via_slash

get_files() — Arvados bugfix

Fix: Filter logic was broken — independent if blocks caused duplicate results, and a misplaced else (attached to the suffix check, not the overall block) appended unfiltered files. Rewrote as skip-on-mismatch (continue) pattern. Additionally, it now returns the fully nested subdir path as opposed to just the collection name with the file name appended (correctly can return /inputs/run1/sample1/file1.fq.gz as opposed to the incorrect /inputs/file1.fq.gz previously).


copy_file() — All platforms

Platform Behavior
SevenBridges Creates target folder path if file_path provided, uses file.copy() or file.copy_to_folder()
Arvados Parses source keep URI, finds/creates destination collection (named from first path component), copies via Collection.copy()
NGS360 Logs warning, returns file unchanged (WES doesn't support file copy)

_list_files_in_folder() — SevenBridges

Fix: When _find_or_create_path returns None (root path), falls back to self.api.files.query(project=project).all() instead of failing with AttributeError on None.id.


get_current_user() — All platforms

Platform Implementation
SevenBridges self.api.users.me() → maps to standard dict
Arvados self.api.users().current().execute() → maps fields
NGS360 GET /api/v1/auth/me with Bearer token; parses UserPublic response directly (no "data" wrapper); safely handles null full_name

@yfan2012
yfan2012 marked this pull request as ready for review June 15, 2026 17:39
@yfan2012
yfan2012 requested review from accumb3ns and golharam June 15, 2026 17:40
Comment thread src/cwl_platform/base_platform.py Outdated
Comment thread src/cwl_platform/base_platform.py Outdated
Comment thread src/cwl_platform/base_platform.py Outdated
Comment thread src/cwl_platform/ngs360_platform.py Outdated
Comment thread src/cwl_platform/ngs360_platform.py
Comment thread src/cwl_platform/sevenbridges_platform.py
Comment thread src/cwl_platform/sevenbridges_platform.py
Comment on lines -297 to -301
if filters and 'suffix' in filters:
if f.name.endswith(filters['suffix']):
matching_files.append((f'/{collection_name}/{f.name}', file_id))
else:
matching_files.append((f'/{collection_name}/{f.name}', file_id))

@yfan2012 yfan2012 Jun 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/else pairing causes all files to be matched as long as the 'suffix' is not specified, but another filter is. I think this is incorrect. See test_filters_work_on_both_methods() in test_get_files_fullpath.py

@yfan2012

yfan2012 commented Jun 22, 2026

Copy link
Copy Markdown
Author

Regarding previous testing for the get_files() method with nested dir structure:

  • This method previously did not have any unit tests - only integration tests requiring a live connection
  • The integration tests only covered cases where a file was uploaded into a collection with no nested dir structure - this is why the bug was missed by the tests.
  • Note: all the major launchers (RNA-seq, WES, WGS) primarily use get_file_id(), so they are unaffected by the bug. scRNA-Seq, gRNA, HIVE, etc use thier own versions of the PAML, so are also unaffected.

New changes:

  • integration tests now check cases with nesting dir structure
  • get_files_fullpath() removed, and its more correct behaviour that reports the full path is now incorporated into get_files()
  • For SBG, copy_file() is now also updated to natively change any potential file_id inputs into file objects (following the same pattern of download_file() and export_file())

Comment thread src/cwl_platform/arvados_platform.py
Comment thread src/cwl_platform/base_platform.py Outdated
:return: List of tuples (file path, file object) matching filter criteria
:return: List of tuples (full_path, file_ref) matching filter criteria.
full_path: The complete path including collection/folder and subdirectories
e.g., "/CollectionName/subdir/file.fastq.gz"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CollectionName shouldn't be mentioned here in base_platform.py. That's Arvados specific.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the example now given is the more realistic /inputs/run1/sample1/file1.fastq.gz

@yfan2012

Copy link
Copy Markdown
Author

Tested RNA/WES/WGS pipeline launchers with these changes in Arvados and SBG using P-00000000-0001. All behaving normally.

@golharam
golharam merged commit cb560ae into main Jun 30, 2026
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants