Typings added #12884

dakariakin · 2021-11-09T14:35:21Z

Why: common-workflow-lab#137
What: Improved type hints

How to test the changes?

(Select all options that apply)

I've included appropriate automated tests.
This is a refactoring of components with existing test coverage.
Instructions for manual testing are as follows:
1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

I agree to license these contributions under Galaxy's current license.
I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to galaxy-committers@lists.galaxyproject.org.

mvdbeek

Thanks a lot @dakariakin!

lib/galaxy/model/dataset_collections/matching.py

lib/galaxy/job_execution/output_collect.py

lib/galaxy/model/dataset_collections/matching.py

lib/galaxy/jobs/__init__.py

lib/galaxy/util/bunch.py

test/unit/app/jobs/test_mapper.py

dakariakin · 2021-11-11T20:43:52Z

lib/galaxy/jobs/__init__.py

@@ -579,9 +588,7 @@ def __set_default_job_conf(self):
            self._set_default_handler_assignment_methods()
        else:
            self.app.application_stack.init_job_handling(self)


these two lines moved to __init__

I don't think that works ... it is definitely a code smell and we should have this in __init__, but the handler setup is really complicated.

Should we create a ticket to refactor this?

Should we create a ticket to refactor this?

Yes, please

Done #12970

dakariakin · 2021-11-11T20:46:10Z

lib/galaxy/jobs/__init__.py

@@ -1228,7 +1238,7 @@ def working_directory(self):

    def working_directory_exists(self):
        job = self.get_job()
-        return self.app.object_store.exists(job, base_dir='job_work', dir_only=True, obj_dir=True)


from lib/galaxy/objectstore/init.py

class ObjectStore ... def exists(self, obj, base_dir=None, dir_only=False, extra_dir=None, extra_dir_at_root=False, alt_name=None):

does not have obj_dir

That's the abstractmethod, but the implementation accepts **kwargs and defers to _exists, which calls _construct_path which does accept obj_dir. I think the fix here is to change the abstractmethod's signature

Updated, thank you!

dakariakin · 2021-11-11T20:46:52Z

lib/galaxy/jobs/__init__.py

@@ -1970,28 +1980,28 @@ def get_output_basenames(self):
        return [os.path.basename(str(fname)) for fname in self.get_output_fnames()]

    def get_output_fnames(self):
-        if self.output_paths is None:


output_paths is [] now instead of None

This is a caching technique: You set the unset value to None, and then you call an expensive method that generates something that is not None and you store the result of that method. Next time you call the method you know you don't need to re-run the method because self.output_paths is not None. But there may be cases where compute_outputs generates an empty list, in which case we don't want to re-run compute_paths, and that breaks here.

Got it, thank you! I've changed it back, please take a look

dakariakin · 2021-11-11T20:48:19Z

lib/galaxy/jobs/runners/local.py

@@ -105,7 +105,6 @@ def queue_job(self, job_wrapper):
                                    env=self._environ,
                                    preexec_fn=os.setpgrp)

-            proc.terminated_by_shutdown = False


subprocess.Popen doesn't have terminated_by_shutdown
and only 1 usage:
if proc.terminated_by_shutdown:

Actually, after rebase on the latest galaxy#dev branch I found that terminated_by_shutdown is used more than once. and also I have a failed test because of that. so I returned it back and ignore the type because I'm not sure how I can add type to terminated_by_shutdown

dakariakin · 2021-11-11T20:49:15Z

lib/galaxy/tool_util/client/staging.py

@@ -117,7 +117,7 @@ def _attach_file(upload_payload, uri, index=0):
            elif isinstance(upload_target, DirectoryUploadTarget):
                fetch_payload = _fetch_payload(history_id, file_type="directory")
                fetch_payload["targets"][0].pop("elements")
-                tar_path = upload_target.path


DirectoryUploadTarget have only tar_path

dakariakin · 2021-11-11T20:51:27Z

lib/galaxy/tool_util/parser/cwl.py

@@ -134,7 +138,7 @@ def _parse_output(self, tool, output_instance):
        output.metadata_source = ""
        output.parent = None
        output.label = None
-        output.count = None
+        output.count = 1


count initialized as output_dict.get("count", 1) in other places, probably good here too

dakariakin · 2021-11-11T20:54:20Z

lib/galaxy/tool_util/parser/yaml.py

@@ -220,7 +224,11 @@ def _parse_test(i, test_dict):
            name = output["name"]
            value = output.get("file", None)
            attributes = output
-            new_outputs.append((name, value, attributes))


it's bug from here
bb7de2c#diff-5658cc0310b132947709c5165a4d05ac8275afd9a49a7a8ae480661eaccedb93L205

mvdbeek

Very nice, thanks so much @dakariakin!

mvdbeek · 2021-11-15T09:49:05Z

lib/galaxy/jobs/__init__.py

@@ -579,9 +588,7 @@ def __set_default_job_conf(self):
            self._set_default_handler_assignment_methods()
        else:
            self.app.application_stack.init_job_handling(self)


I don't think that works ... it is definitely a code smell and we should have this in __init__, but the handler setup is really complicated.

mvdbeek · 2021-11-15T09:52:27Z

lib/galaxy/jobs/__init__.py

@@ -1228,7 +1238,7 @@ def working_directory(self):

    def working_directory_exists(self):
        job = self.get_job()
-        return self.app.object_store.exists(job, base_dir='job_work', dir_only=True, obj_dir=True)


That's the abstractmethod, but the implementation accepts **kwargs and defers to _exists, which calls _construct_path which does accept obj_dir. I think the fix here is to change the abstractmethod's signature

mvdbeek · 2021-11-15T09:55:58Z

lib/galaxy/jobs/__init__.py

@@ -1970,28 +1980,28 @@ def get_output_basenames(self):
        return [os.path.basename(str(fname)) for fname in self.get_output_fnames()]

    def get_output_fnames(self):
-        if self.output_paths is None:


This is a caching technique: You set the unset value to None, and then you call an expensive method that generates something that is not None and you store the result of that method. Next time you call the method you know you don't need to re-run the method because self.output_paths is not None. But there may be cases where compute_outputs generates an empty list, in which case we don't want to re-run compute_paths, and that breaks here.

mr-c · 2021-11-22T13:37:50Z

lib/galaxy/jobs/runners/local.py

@@ -105,7 +105,7 @@ def queue_job(self, job_wrapper):
                                    env=self._environ,
                                    preexec_fn=os.setpgrp)

-            proc.terminated_by_shutdown = False
+            proc.terminated_by_shutdown = False  # type: ignore


now that we have show_error_codes = True under [mypy] in setup.cfg, you can refine these # type: ignore to include the specific error code being ignored. Example: # type: ignore[attr-defined]

I've updated it, thank you!

mr-c · 2021-11-22T13:39:07Z

lib/galaxy/model/dataset_collections/matching.py

+        if self.linked_structure:
+            return self.linked_structure.walk_collections(self.collections)
+        else:
+            return []


I think raising an Exception would be better instead of silently returning an empty set, yes?

Changed back to assert. I think it could break some tests, but let's see

dakariakin · 2022-10-10T16:27:48Z

The original issue is closed common-workflow-lab#137

mr-c · 2022-10-10T21:15:05Z

The original issue is closed common-workflow-lab#137

Oh, I didn't mean that this PR was no longer needed! Can you reopen this?

dakariakin · 2022-10-11T14:04:57Z

The original issue is closed common-workflow-lab#137

Oh, I didn't mean that this PR was no longer needed! Can you reopen this?

Sorry, I thought it was done in some other PR. I'll rebase this PR and prepare it for a code review

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>

Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>

dakariakin mentioned this pull request Nov 9, 2021

Enhance the Python typing for the files we touch common-workflow-lab/galaxy#137

Closed

57 tasks

mvdbeek reviewed Nov 9, 2021

View reviewed changes

lib/galaxy/model/dataset_collections/matching.py Outdated Show resolved Hide resolved

lib/galaxy/job_execution/output_collect.py Outdated Show resolved Hide resolved

nsoranzo added BioHackEU21 kind/enhancement labels Nov 9, 2021

nsoranzo added this to the 22.01 milestone Nov 9, 2021

nsoranzo reviewed Nov 10, 2021

View reviewed changes

lib/galaxy/job_execution/output_collect.py Outdated Show resolved Hide resolved

lib/galaxy/model/dataset_collections/matching.py Outdated Show resolved Hide resolved

nsoranzo reviewed Nov 10, 2021

View reviewed changes

lib/galaxy/jobs/__init__.py Outdated Show resolved Hide resolved

lib/galaxy/util/bunch.py Outdated Show resolved Hide resolved

test/unit/app/jobs/test_mapper.py Outdated Show resolved Hide resolved

mvdbeek mentioned this pull request Nov 10, 2021

Type updates #12868

Merged

5 tasks

dakariakin force-pushed the added_types branch from f49271e to 2f725bd Compare November 11, 2021 20:42

dakariakin commented Nov 11, 2021

View reviewed changes

dakariakin changed the title ~~WIP: Added types~~ Typings added Nov 11, 2021

github-actions bot added area/jobs area/testing area/tool-framework area/util labels Nov 11, 2021

dakariakin marked this pull request as ready for review November 11, 2021 20:56

mvdbeek reviewed Nov 15, 2021

View reviewed changes

dakariakin force-pushed the added_types branch from ef06245 to e528213 Compare November 16, 2021 15:04

mr-c reviewed Nov 22, 2021

View reviewed changes

dakariakin force-pushed the added_types branch from e528213 to 156b93e Compare November 23, 2021 20:34

mvdbeek modified the milestones: 22.01, 22.05 Jan 14, 2022

mvdbeek added the status/WIP label Jan 14, 2022

mvdbeek removed this from the 22.05 milestone May 25, 2022

nsoranzo marked this pull request as draft September 16, 2022 00:09

nsoranzo removed the status/WIP label Sep 16, 2022

dakariakin closed this Oct 10, 2022

nsoranzo reopened this Oct 10, 2022

dakariakin and others added 18 commits October 24, 2022 13:14

added types to lib/galaxy/model/dataset_collections/types/__init__.py

01833c1

added types to lib/galaxy/model/dataset_collections/matching.py

a9d3795

added types to lib/galaxy/job_execution/output_collect.py

2e2501f

Apply suggestions from code review

6e9103e

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>

Apply suggestions from code review

3e81431

Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>

added types to lib/galaxy/jobs/__init__.py

2008c00

added types to lib/galaxy/jobs/runners/local.py

9b05bf1

fixed types errors in lib/galaxy/tool_util/output_checker.py

c113686

Apply suggestions from code review

9882799

Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>

added types to lib/galaxy/tool_util/cwl/parser.py

f990b9a

added types to lib/galaxy/tool_util/cwl/representation.py

0a21a82

added types to lib/galaxy/tool_util/parser/yaml.py

e7f4810

added types to lib/galaxy/tool_util/parser/output_objects.py

83a054c

added types to lib/galaxy/metadata/__init__.py

9f3643c

Fix linter errors

c1974fe

code review updates and test fixes

b4315cf

Code review updates: add error codes to ignore

a31dc61

Fixes after rebase

f938bf6

dakariakin force-pushed the added_types branch from 156b93e to f938bf6 Compare October 24, 2022 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typings added #12884

Typings added #12884

dakariakin commented Nov 9, 2021

mvdbeek left a comment

dakariakin Nov 11, 2021

mvdbeek Nov 15, 2021

dakariakin Nov 16, 2021

mr-c Nov 22, 2021

dakariakin Nov 23, 2021

dakariakin Nov 11, 2021

mvdbeek Nov 15, 2021

dakariakin Nov 16, 2021

dakariakin Nov 11, 2021

mvdbeek Nov 15, 2021

dakariakin Nov 16, 2021

dakariakin Nov 11, 2021

dakariakin Nov 16, 2021

dakariakin Nov 11, 2021

dakariakin Nov 11, 2021

dakariakin Nov 11, 2021

mvdbeek left a comment

mvdbeek Nov 15, 2021

mvdbeek Nov 15, 2021

mvdbeek Nov 15, 2021

mr-c Nov 22, 2021

dakariakin Nov 23, 2021

mr-c Nov 22, 2021

dakariakin Nov 23, 2021

dakariakin commented Oct 10, 2022

mr-c commented Oct 10, 2022

dakariakin commented Oct 11, 2022

Typings added #12884

Are you sure you want to change the base?

Typings added #12884

Conversation

dakariakin commented Nov 9, 2021

How to test the changes?

License

mvdbeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvdbeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakariakin commented Oct 10, 2022

mr-c commented Oct 10, 2022

dakariakin commented Oct 11, 2022