Improve type hints in manager classes #18129

mvdbeek · 2024-05-13T07:58:42Z

Most of the return values of the central managers now have type annotations, which has already caught a couple of minor issues (4fdcb1b, 77af0f6 and c8d4f14)

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 and all my past contributions to the core galaxy codebase under the MIT license.

davelopez

Wonderful!
Feel free to ignore some of the type hint suggestions if they create more issues than they are worth.

lib/galaxy/managers/datasets.py

lib/galaxy/managers/users.py

davelopez · 2024-05-14T09:45:18Z

lib/galaxy/model/__init__.py

@@ -732,7 +732,7 @@ class User(Base, Dictifiable, RepresentById):
    create_time: Mapped[datetime] = mapped_column(default=now, nullable=True)
    update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True)
    email: Mapped[str] = mapped_column(TrimmedString(255), index=True)
-    username: Mapped[Optional[str]] = mapped_column(TrimmedString(255), index=True, unique=True)
+    username: Mapped[str] = mapped_column(TrimmedString(255), index=True, unique=True)


If I understood correctly the third bullet point in #17958, we need to add nullable=True to mapped_column to reflect the real table definition.

The same applies to the rest of the model attributes that changed to non-optional type hints here.

Hehe, I was looking for that, it would have saved me from changing all those unit tests 😆

This means that when we list/filter/retrieve items from a given manager we get the correct return type, instead of `any`.

…anager

The model optionality on the sqlalchemy side seems weird.

Co-authored-by: David López <[email protected]>

This allows type lookups using TypedDict, which isn't possible with enum members.

… can't be opened

lib/galaxy/model/__init__.py

Co-authored-by: David López <[email protected]>

mvdbeek · 2024-05-24T06:54:39Z

The one failing test is unrelated.

davelopez

I love all these refactorings! thank you very much!

Just a couple of comments, one I think might be an ugly bug about archived (non-purged) histories 😞

davelopez · 2024-05-24T07:48:28Z

lib/galaxy/managers/users.py

+            raise exceptions.MessageException(f"User '{item.email}' has not been deleted, so they cannot be purged.")
+        private_role = self.app.security_agent.get_private_user_role(item)
+        # Delete History
+        for active_history in item.active_histories:


I've just noticed that this probably will not purge archived histories from the user... 😬 😞

davelopez · 2024-05-24T07:57:10Z

lib/galaxy/schema/schema.py

-    description="Whether this dataset belongs to a history (HDA) or a library (LDDA).",
-)
+HdaLddaField = Annotated[
+    DataItemSourceType,


I think what we want here is DatasetSourceType 🤔

Suggested change

DataItemSourceType,

DatasetSourceType,

lib/galaxy/webapps/galaxy/services/datasets.py

davelopez · 2024-05-24T08:14:37Z

lib/galaxy/webapps/galaxy/services/datasets.py

+                    hda_manager.error_if_uploading(hda)
+                    hda_manager.error_unless_mutable(hda.history)


I wonder if one more dictionary called raise_delete_error_by_type that will call these 2 methods for hdas and just no-op for lddas will eliminate this duplication 🤔

mvdbeek · 2024-11-28T14:32:59Z

This is so heavily conflicted now it probably doesn't make sense to keep around.

mvdbeek added the kind/refactoring cleanup or refactoring of existing code, no functional changes label May 13, 2024

github-actions bot added area/testing area/API area/database Galaxy's database or data access layer area/tool-framework area/testing/integration labels May 13, 2024

github-actions bot added this to the 24.1 milestone May 13, 2024

mvdbeek force-pushed the improve_type_hints_in_manager_classes branch 2 times, most recently from 392da47 to c22dfd8 Compare May 14, 2024 08:54

This was referenced May 14, 2024

[24.0] Don't commit in DeleteIntermediatesAction #18131

Merged

[24.0] Fix data default values not getting added to history #18132

Merged

mvdbeek force-pushed the improve_type_hints_in_manager_classes branch from c22dfd8 to baf7fef Compare May 14, 2024 09:25

davelopez reviewed May 14, 2024

View reviewed changes

mvdbeek removed this from the 24.1 milestone May 14, 2024

mvdbeek force-pushed the improve_type_hints_in_manager_classes branch from baf7fef to 7f9a46c Compare May 21, 2024 12:55

mvdbeek and others added 15 commits May 23, 2024 17:17

Make most base managers generic and parameterize concrete item managers

b70a23b

This means that when we list/filter/retrieve items from a given manager we get the correct return type, instead of `any`.

Split UserManager into shared base manager and galaxy-specific user m…

76b7288

…anager

Add ToolShed specific user manager

d116ea9

Fix bugs and type hints reveealed by improved type hints

d4b2cc5

Drop unnecessary type ignores

6d046f5

Make ApiKeyManager generic for galaxy or ts models

d32b6dd

Fix and adjust model type optionality

f82bdd7

The model optionality on the sqlalchemy side seems weird.

Cannot use union in lagom

ce73553

Fix type annotation for api_keys.key

fed8b1a

Fix nullability of TS api key

668deb2

Fixup setup_users in celery rate limit integration test

3076954

Fix unit tests

9866f4e

Add more type hints in manager signatures

1aedb85

Co-authored-by: David López <[email protected]>

Fix up history pruner unit test

da79c31

Ensure dataset on disk befoe access

a1760cc

mvdbeek added 14 commits May 23, 2024 17:20

Tighten up use of DatasetProtocol and DatasetHasHidProtocol

2523f47

Make AccessibleManagerMixin generic

5b856b5

Specialize generated query in ModelManager

1c61242

Add type parameters to DatasetManager

67024f0

Add type annotation to HDAManager copy signature

f388f05

Eliminate unnecessary type ignores

e8d0e09

Add type hint for dataset_manager_by_type

35b1e12

Undo DatasetSourceType change

b5e1ddc

Turn DatasetSourceType into literal

f5ed1f1

This allows type lookups using TypedDict, which isn't possible with enum members.

Do proper type narrowing in delete_batch

57f0085

Ensure dataset should be on disk before opening dataset

796fcd1

Remove unnecessary by_id implementation

26b6c74

Add API test to ensure that a proper error is returned if the dataset…

0eced90

… can't be opened

Restore nullability of column definition

70aa9d7

mvdbeek force-pushed the improve_type_hints_in_manager_classes branch from 81d7635 to 70aa9d7 Compare May 23, 2024 15:20

davelopez reviewed May 23, 2024

View reviewed changes

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

Fix nullable typo

2e0fe2a

Co-authored-by: David López <[email protected]>

mvdbeek requested a review from a team May 24, 2024 06:54

davelopez reviewed May 24, 2024

View reviewed changes

davelopez mentioned this pull request May 24, 2024

[24.0] Check dataset state when attempting to acces dataset contents #18214

Merged

4 tasks

mvdbeek closed this Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve type hints in manager classes #18129

Improve type hints in manager classes #18129

mvdbeek commented May 13, 2024

davelopez left a comment

davelopez May 14, 2024

mvdbeek May 14, 2024

mvdbeek commented May 24, 2024

davelopez left a comment

davelopez May 24, 2024

davelopez May 24, 2024

davelopez May 24, 2024

mvdbeek commented Nov 28, 2024

		hda_manager.error_if_uploading(hda)
		hda_manager.error_unless_mutable(hda.history)

Improve type hints in manager classes #18129

Improve type hints in manager classes #18129

Conversation

mvdbeek commented May 13, 2024

How to test the changes?

License

davelopez left a comment

Choose a reason for hiding this comment

davelopez May 14, 2024

Choose a reason for hiding this comment

mvdbeek May 14, 2024

Choose a reason for hiding this comment

mvdbeek commented May 24, 2024

davelopez left a comment

Choose a reason for hiding this comment

davelopez May 24, 2024

Choose a reason for hiding this comment

davelopez May 24, 2024

Choose a reason for hiding this comment

davelopez May 24, 2024

Choose a reason for hiding this comment

mvdbeek commented Nov 28, 2024