From 320867ea0d83d96f4d9ba39e111aa7d7d0a87bef Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Thu, 24 Apr 2025 13:58:48 -0400 Subject: [PATCH 01/12] reconfigured abstractmethod usage --- isabl_cli/app.py | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 36093af..17c691b 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -29,7 +29,7 @@ from isabl_cli.settings import system_settings -class AbstractApplication: # pylint: disable=too-many-public-methods +class AbstractApplication(abc.ABC): # pylint: disable=too-many-public-methods """An Abstract Isabl application.""" @@ -147,7 +147,7 @@ class AbstractApplication: # pylint: disable=too-many-public-methods # ----------------------------- # USER REQUIRED IMPLEMENTATIONS # ----------------------------- - + @abc.abstractmethod def get_command(self, analysis, inputs, settings): # pylint: disable=W9008 """ Must return a shell command for the analysis as a string. @@ -166,7 +166,6 @@ def get_command(self, analysis, inputs, settings): # pylint: disable=W9008 # OPTIONAL IMPLEMENTATIONS # ------------------------ - @abc.abstractmethod def get_experiments_from_cli_options(self, **cli_options): # pylint: disable=W9008 """ Must return list of target-reference experiment tuples given the parsed options. @@ -261,7 +260,6 @@ def get_project_analysis_results(self, analysis): # pylint: disable=W9008,W0613 """ return {} # pragma: no cover - @abc.abstractmethod # add __isabstractmethod__ property to method def merge_project_analyses(self, analysis, analyses): """ Merge analyses on a project level basis. @@ -298,7 +296,6 @@ def get_individual_analysis_results(self, analysis): # pylint: disable=W9008,W0 """ return {} # pragma: no cover - @abc.abstractmethod # add __isabstractmethod__ property to method def merge_individual_analyses(self, analysis, analyses): """ Merge analyses on a individual level basis. @@ -594,9 +591,10 @@ def application(self): @cached_property def individual_level_auto_merge_application(self): """Get or create an individual level application database object.""" - assert not hasattr( - self.merge_individual_analyses, "__isabstractmethod__" - ), "No logic implemented to merge analyses for an individual..." + if not self.is_overridden(self.merge_individual_analyses.__name__): + raise NotImplementedError( + "No logic implemented to merge analyses for an individual..." + ) application = api.create_instance( endpoint="applications", @@ -617,10 +615,11 @@ def individual_level_auto_merge_application(self): @cached_property def project_level_auto_merge_application(self): """Get or create a project level application database object.""" - assert not hasattr( - self.merge_project_analyses, "__isabstractmethod__" - ), "No logic implemented to merge project analyses..." - + if not self.is_overridden(self.merge_project_analyses.__name__): + raise NotImplementedError( + "No logic implemented to merge analyses for a project..." + ) + application = api.create_instance( endpoint="applications", name=f"{self.NAME} Project Application", @@ -645,12 +644,15 @@ def assembly(self): @property def has_project_auto_merge(self): """Return True if project level auto merge logic is defined.""" - return not hasattr(self.merge_project_analyses, "__isabstractmethod__") + if not self.is_overridden(self.merge_project_analyses.__name__): + raise NotImplementedError( + "No logic implemented to merge analyses for a project..." + ) @property def has_individual_auto_merge(self): - """Return True if individual level auto merge logic is defined.""" - return not hasattr(self.merge_individual_analyses, "__isabstractmethod__") + """Return True if individual level auto merge logic is defined.""" + return self.is_overridden(self.merge_individual_analyses.__name__) @property def _application_results(self): @@ -1241,6 +1243,12 @@ def _get_analysis_results(self, analysis, created=False): # APPLICATION UTILS # ----------------- + @staticmethod + def is_overridden(self, method_name): + base_method = getattr(AbstractApplication, method_name, None) + instance_method = getattr(self, method_name, None) + return instance_method is not None and instance_method.__func__ is not base_method + @staticmethod def get_result(*args, **kwargs): # pragma: no cover """Get an application result.""" From e3d5c704727c8af57a844e7307d987aa14e2e2b5 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Thu, 24 Apr 2025 14:19:59 -0400 Subject: [PATCH 02/12] more general implementation of is_overridden --- isabl_cli/app.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 17c691b..5e49ee0 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -591,7 +591,7 @@ def application(self): @cached_property def individual_level_auto_merge_application(self): """Get or create an individual level application database object.""" - if not self.is_overridden(self.merge_individual_analyses.__name__): + if not self.is_overridden(self.merge_individual_analyses): raise NotImplementedError( "No logic implemented to merge analyses for an individual..." ) @@ -615,7 +615,7 @@ def individual_level_auto_merge_application(self): @cached_property def project_level_auto_merge_application(self): """Get or create a project level application database object.""" - if not self.is_overridden(self.merge_project_analyses.__name__): + if not self.is_overridden(self.merge_project_analyses): raise NotImplementedError( "No logic implemented to merge analyses for a project..." ) @@ -644,7 +644,7 @@ def assembly(self): @property def has_project_auto_merge(self): """Return True if project level auto merge logic is defined.""" - if not self.is_overridden(self.merge_project_analyses.__name__): + if not self.is_overridden(self.merge_project_analyses): raise NotImplementedError( "No logic implemented to merge analyses for a project..." ) @@ -652,7 +652,7 @@ def has_project_auto_merge(self): @property def has_individual_auto_merge(self): """Return True if individual level auto merge logic is defined.""" - return self.is_overridden(self.merge_individual_analyses.__name__) + return self.is_overridden(self.merge_individual_analyses) @property def _application_results(self): @@ -1243,11 +1243,12 @@ def _get_analysis_results(self, analysis, created=False): # APPLICATION UTILS # ----------------- - @staticmethod - def is_overridden(self, method_name): - base_method = getattr(AbstractApplication, method_name, None) - instance_method = getattr(self, method_name, None) - return instance_method is not None and instance_method.__func__ is not base_method + def is_overridden(self, method): + """Checks if a method of the base class was overridden.""" + method_name = method.__name__ + base_method = getattr(self.__class__.__bases__[0], method_name, None) + return method.__func__ is not base_method + @staticmethod def get_result(*args, **kwargs): # pragma: no cover From 9dae15d1663e275ad9bb83adb7d6b1cee1d5aa0f Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Thu, 24 Apr 2025 14:24:50 -0400 Subject: [PATCH 03/12] hard code is_overridden to check AbstractApplication --- isabl_cli/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 5e49ee0..0cae9d3 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -1246,7 +1246,7 @@ def _get_analysis_results(self, analysis, created=False): def is_overridden(self, method): """Checks if a method of the base class was overridden.""" method_name = method.__name__ - base_method = getattr(self.__class__.__bases__[0], method_name, None) + base_method = getattr(AbstractApplication, method_name, None) return method.__func__ is not base_method From d29effa4542d86e402087f621802c50c9bbb1bd9 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Thu, 24 Apr 2025 14:56:24 -0400 Subject: [PATCH 04/12] replace AbstractApplication() with BaseMockApplication() so tests pass --- tests/test_app.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/test_app.py b/tests/test_app.py index a01a575..583f451 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -583,7 +583,7 @@ def test_engine(tmpdir): def test_validate_is_pair(): - application = AbstractApplication() + application = BaseMockApplication() application.validate_is_pair([{"pk": 1}], [{"pk": 2}]) with pytest.raises(AssertionError) as error: @@ -600,7 +600,7 @@ def test_validate_is_pair(): def test_validate_reference_genome(tmpdir): reference = tmpdir.join("reference.fasta") required = ".fai", ".amb", ".ann", ".bwt", ".pac", ".sa" - application = AbstractApplication() + application = BaseMockApplication() with pytest.raises(AssertionError) as error: application.validate_reference_genome(reference.strpath) @@ -618,7 +618,7 @@ def test_validate_reference_genome(tmpdir): def test_validate_fastq_only(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"raw_data": [], "system_id": "FOO"}] with pytest.raises(AssertionError) as error: @@ -645,7 +645,7 @@ def test_validate_fastq_only(): def test_validate_methods(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"technique": {"method": "FOO"}, "system_id": "FOO BAR"}] with pytest.raises(AssertionError) as error: @@ -655,7 +655,7 @@ def test_validate_methods(): def test_validate_pdx_only(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"custom_fields": {"is_pdx": False}, "system_id": "FOO"}] with pytest.raises(AssertionError) as error: @@ -665,7 +665,7 @@ def test_validate_pdx_only(): def test_validate_are_normals(): - application = AbstractApplication() + application = BaseMockApplication() targets = [ api.isablfy( {"sample": {"category": "TUMOR", "system_id": "FOO"}, "system_id": "FOO"} @@ -679,7 +679,7 @@ def test_validate_are_normals(): def test_validate_dna_rna_only(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"technique": {"category": "DNA"}, "system_id": "FOO"}] with pytest.raises(AssertionError) as error: @@ -706,7 +706,7 @@ def test_validate_species(): def test_validate_one_target_no_references(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{}] references = [] application.validate_one_target_no_references(targets, references) @@ -719,7 +719,7 @@ def test_validate_one_target_no_references(): def test_validate_atleast_onetarget_onereference(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{}] references = [{}] application.validate_at_least_one_target_one_reference(targets, references) @@ -732,7 +732,7 @@ def test_validate_atleast_onetarget_onereference(): def test_validate_targets_not_in_references(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"pk": 1, "system_id": 1}] references = [{"pk": 2, "system_id": 2}] application.validate_targets_not_in_references(targets, references) @@ -745,7 +745,7 @@ def test_validate_targets_not_in_references(): def test_validate_dna_tuples(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"system_id": 1, "technique": {"category": "DNA"}}] references = [{"system_id": 2, "technique": {"category": "DNA"}}] application.validate_dna_only(targets + references) @@ -758,14 +758,14 @@ def test_validate_dna_tuples(): def test_validate_dna_pairs(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"pk": 1, "technique": {"category": "DNA"}}] references = [{"pk": 2, "technique": {"category": "DNA"}}] application.validate_dna_pairs(targets, references) def test_validate_same_technique(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"system_id": 1, "technique": {"slug": "1"}}] references = [{"system_id": 2, "technique": {"slug": "1"}}] application.validate_same_technique(targets, references) @@ -784,7 +784,7 @@ def test_validate_same_technique(): def test_validate_same_platform(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"system_id": 1, "platform": {"slug": "1"}}] references = [{"system_id": 2, "platform": {"slug": "1"}}] application.validate_same_platform(targets, references) @@ -803,7 +803,7 @@ def test_validate_same_platform(): def test_validate_source(): - application = AbstractApplication() + application = BaseMockApplication() targets = [{"sample": {"system_id": "FOO", "source": "BLOOD"}}] application.validate_source(targets, "BLOOD") @@ -877,7 +877,7 @@ def test_get_experiments_from_default_cli_options(tmpdir): def test_validate_individuals(): # Test matched analyis - matched_application = AbstractApplication() + matched_application = BaseMockApplication() targets = [ {"system_id": 1, "sample": {"individual": {"pk": 1, "system_id": "ind1"}}} @@ -895,7 +895,7 @@ def test_validate_individuals(): assert "Same individual required:" in str(error.value) # Test unmatched analysis - unmatched_application = AbstractApplication() + unmatched_application = BaseMockApplication() unmatched_application.IS_UNMATCHED = True targets = [ From 4a5a6f5e3520623de2e62364caf30b888cd78bae Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 14:28:43 -0400 Subject: [PATCH 05/12] missed one is_overridden replacement --- isabl_cli/app.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 0cae9d3..0cc185d 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -761,10 +761,8 @@ def command(commit, quiet, **cli_options): if force and restart: raise click.UsageError("cant use --force and --restart together") - - if not hasattr( - cls.get_experiments_from_cli_options, "__isabstractmethod__" - ): + + if not cls.is_overridden(cls.get_experiments_from_cli_options): tuples = pipe.get_experiments_from_cli_options(**cli_options) else: tuples = cls.get_experiments_from_default_cli_options(cli_options) From 76e8269a8b7844205ccdacf1e9aa1f7d92bbb7b3 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 14:32:17 -0400 Subject: [PATCH 06/12] get_command for NonSequencingApplication --- tests/test_app.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_app.py b/tests/test_app.py index 583f451..d1d4490 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -19,6 +19,10 @@ class NonSequencingApplication(AbstractApplication): NAME = str(uuid.uuid4()) VERSION = "STILL_TESTING" + def get_command(*_): # pylint: disable=no-method-argument + return "echo instantiated without an assembly" + + class ExperimentsFromDefaulCLIApplication(AbstractApplication): NAME = str(uuid.uuid4()) From 8dc846d4e2328ee9fce254fbde38a8ef5dcacc12 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 14:38:21 -0400 Subject: [PATCH 07/12] fix copypasta --- isabl_cli/app.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 0cc185d..b0b6a22 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -644,10 +644,7 @@ def assembly(self): @property def has_project_auto_merge(self): """Return True if project level auto merge logic is defined.""" - if not self.is_overridden(self.merge_project_analyses): - raise NotImplementedError( - "No logic implemented to merge analyses for a project..." - ) + return self.is_overridden(self.merge_project_analyses) @property def has_individual_auto_merge(self): From 388b9763e700413a4f501132833fe4ef8a0ec673 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 14:43:45 -0400 Subject: [PATCH 08/12] rename is_overridden to is_implemented --- isabl_cli/app.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index b0b6a22..e7a9797 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -591,7 +591,7 @@ def application(self): @cached_property def individual_level_auto_merge_application(self): """Get or create an individual level application database object.""" - if not self.is_overridden(self.merge_individual_analyses): + if not self.is_implemented(self.merge_individual_analyses): raise NotImplementedError( "No logic implemented to merge analyses for an individual..." ) @@ -615,7 +615,7 @@ def individual_level_auto_merge_application(self): @cached_property def project_level_auto_merge_application(self): """Get or create a project level application database object.""" - if not self.is_overridden(self.merge_project_analyses): + if not self.is_implemented(self.merge_project_analyses): raise NotImplementedError( "No logic implemented to merge analyses for a project..." ) @@ -644,12 +644,12 @@ def assembly(self): @property def has_project_auto_merge(self): """Return True if project level auto merge logic is defined.""" - return self.is_overridden(self.merge_project_analyses) + return self.is_implemented(self.merge_project_analyses) @property def has_individual_auto_merge(self): """Return True if individual level auto merge logic is defined.""" - return self.is_overridden(self.merge_individual_analyses) + return self.is_implemented(self.merge_individual_analyses) @property def _application_results(self): @@ -759,7 +759,7 @@ def command(commit, quiet, **cli_options): if force and restart: raise click.UsageError("cant use --force and --restart together") - if not cls.is_overridden(cls.get_experiments_from_cli_options): + if not cls.is_implemented(cls.get_experiments_from_cli_options): tuples = pipe.get_experiments_from_cli_options(**cli_options) else: tuples = cls.get_experiments_from_default_cli_options(cli_options) @@ -1238,7 +1238,7 @@ def _get_analysis_results(self, analysis, created=False): # APPLICATION UTILS # ----------------- - def is_overridden(self, method): + def is_implemented(self, method): """Checks if a method of the base class was overridden.""" method_name = method.__name__ base_method = getattr(AbstractApplication, method_name, None) From 4d32179d33fbd2a944c80439ab686f0d5a43e9f5 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 15:11:52 -0400 Subject: [PATCH 09/12] is_implemented is a classmethod --- isabl_cli/app.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index e7a9797..35dd39b 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -1237,11 +1237,12 @@ def _get_analysis_results(self, analysis, created=False): # ----------------- # APPLICATION UTILS # ----------------- - - def is_implemented(self, method): + + @classmethod + def is_implemented(cls, method): """Checks if a method of the base class was overridden.""" method_name = method.__name__ - base_method = getattr(AbstractApplication, method_name, None) + base_method = getattr(AbstractApplication, method_name, None) return method.__func__ is not base_method From 77b58bb0d883280b381c8ff6f0bfdb458d984bf2 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 16:03:05 -0400 Subject: [PATCH 10/12] account for classmethods vs instance methods --- isabl_cli/app.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 35dd39b..993bcc6 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -1242,8 +1242,9 @@ def _get_analysis_results(self, analysis, created=False): def is_implemented(cls, method): """Checks if a method of the base class was overridden.""" method_name = method.__name__ + method_func = getattr(cls, method_name, None) # accounts for classmethods vs instance methods base_method = getattr(AbstractApplication, method_name, None) - return method.__func__ is not base_method + return method_func is not base_method @staticmethod From 90f3453b32eabec9370291d05c9ae415ebdff73c Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 16:24:10 -0400 Subject: [PATCH 11/12] make staticmethod --- isabl_cli/app.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 993bcc6..2c416f7 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -1238,11 +1238,11 @@ def _get_analysis_results(self, analysis, created=False): # APPLICATION UTILS # ----------------- - @classmethod - def is_implemented(cls, method): + @staticmethod + def is_implemented(method): """Checks if a method of the base class was overridden.""" method_name = method.__name__ - method_func = getattr(cls, method_name, None) # accounts for classmethods vs instance methods + method_func = getattr(method, "__func__", method) # accounts for classmethods vs instance methods base_method = getattr(AbstractApplication, method_name, None) return method_func is not base_method From 9daba613b4dd6a78a4854deeb59bb0ab41d84fa9 Mon Sep 17 00:00:00 2001 From: Tobias Schraink Date: Tue, 6 May 2025 16:30:14 -0400 Subject: [PATCH 12/12] fixed incorrect not cls.is_implemented logic --- isabl_cli/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/isabl_cli/app.py b/isabl_cli/app.py index 2c416f7..13ad46a 100644 --- a/isabl_cli/app.py +++ b/isabl_cli/app.py @@ -759,7 +759,7 @@ def command(commit, quiet, **cli_options): if force and restart: raise click.UsageError("cant use --force and --restart together") - if not cls.is_implemented(cls.get_experiments_from_cli_options): + if cls.is_implemented(cls.get_experiments_from_cli_options): tuples = pipe.get_experiments_from_cli_options(**cli_options) else: tuples = cls.get_experiments_from_default_cli_options(cli_options)