Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit superclass to subclass lazy typing #696

Merged
merged 13 commits into from
Feb 27, 2024

Conversation

tclose
Copy link
Contributor

@tclose tclose commented Sep 6, 2023

Types of changes

  • Enhancement

Summary

Relaxes typing of lazy fields to allow super-classes to be passed to sub-class fields. Implements #682

Checklist

  • I have added tests to cover my changes (if necessary)

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.52%. Comparing base (1720ba6) to head (27e7fb8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   83.35%   83.52%   +0.17%     
==========================================
  Files          24       24              
  Lines        4949     4970      +21     
  Branches     1411     1415       +4     
==========================================
+ Hits         4125     4151      +26     
+ Misses        816      812       -4     
+ Partials        8        7       -1     
Flag Coverage Δ
unittests 83.52% <100.00%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tclose tclose marked this pull request as ready for review September 7, 2023 09:07
@effigies effigies closed this Sep 7, 2023
@effigies effigies reopened this Sep 7, 2023
@effigies effigies closed this Sep 8, 2023
@effigies effigies reopened this Sep 8, 2023
@tclose
Copy link
Contributor Author

tclose commented Sep 25, 2023

NB: rebased on master to pick up new slurm test

@djarecka
Copy link
Collaborator

djarecka commented Oct 3, 2023

i've restarted the failing slurm test, perhaps we should extend some time in GA


wf.add( # Generic task
other_specific_task(
in_file=wf.entry.lzout.out.cast(MyOtherFormatX),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this cast a method on fileformats? Four attribute accesses feels like a lot to follow. I would find it more straightforward to have a function:

Suggested change
in_file=wf.entry.lzout.out.cast(MyOtherFormatX),
in_file=pydra.cast(wf.entry.lzout.out, MyOtherFormatX),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't fileformats specific, it can be any class that is able to be "cast", i.e. newclass(oldinstance) is valid code such as Path("mystring"). I'm not averse to your suggested syntax, but since it would only be applicable to lazy fields I think I would find having it as a method more convenient

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Overall this looks good. The pydra.cast() proposal is a bikeshed we can paint another day.

@effigies
Copy link
Contributor

That last commit seems to have broken things.

=================================== FAILURES ===================================
___________________________ test_type_check_nested7 ____________________________
[gw0] linux -- Python 3.12.2 /opt/hostedtoolcache/Python/3.12.2/x64/bin/python

    def test_type_check_nested7():
>       with pytest.raises(TypeError, match="Wrong number of type arguments"):
E       Failed: DID NOT RAISE <class 'TypeError'>

/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/pydra/utils/tests/test_typing.py:159: Failed
_______________________ test_matches_type_tuple_ellipsis _______________________
[gw0] linux -- Python 3.12.2 /opt/hostedtoolcache/Python/3.12.2/x64/bin/python

    def test_matches_type_tuple_ellipsis():
        assert TypeParser.matches_type(ty.Tuple[int], ty.Tuple[int, ...])
        assert TypeParser.matches_type(ty.Tuple[int, int], ty.Tuple[int, ...])
        assert not TypeParser.matches_type(ty.Tuple[int, float], ty.Tuple[int, ...])
>       assert not TypeParser.matches_type(ty.Tuple[int, ...], ty.Tuple[int])
E       AssertionError: assert not True
E        +  where True = <bound method TypeParser.matches_type of <class 'pydra.utils.typing.TypeParser'>>(typing.Tuple[int, ...], typing.Tuple[int])
E        +    where <bound method TypeParser.matches_type of <class 'pydra.utils.typing.TypeParser'>> = TypeParser.matches_type

/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/pydra/utils/tests/test_typing.py:513: AssertionError

@tclose
Copy link
Contributor Author

tclose commented Feb 25, 2024

That last commit seems to have broken things.

=================================== FAILURES ===================================
___________________________ test_type_check_nested7 ____________________________
[gw0] linux -- Python 3.12.2 /opt/hostedtoolcache/Python/3.12.2/x64/bin/python

    def test_type_check_nested7():
>       with pytest.raises(TypeError, match="Wrong number of type arguments"):
E       Failed: DID NOT RAISE <class 'TypeError'>

/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/pydra/utils/tests/test_typing.py:159: Failed
_______________________ test_matches_type_tuple_ellipsis _______________________
[gw0] linux -- Python 3.12.2 /opt/hostedtoolcache/Python/3.12.2/x64/bin/python

    def test_matches_type_tuple_ellipsis():
        assert TypeParser.matches_type(ty.Tuple[int], ty.Tuple[int, ...])
        assert TypeParser.matches_type(ty.Tuple[int, int], ty.Tuple[int, ...])
        assert not TypeParser.matches_type(ty.Tuple[int, float], ty.Tuple[int, ...])
>       assert not TypeParser.matches_type(ty.Tuple[int, ...], ty.Tuple[int])
E       AssertionError: assert not True
E        +  where True = <bound method TypeParser.matches_type of <class 'pydra.utils.typing.TypeParser'>>(typing.Tuple[int, ...], typing.Tuple[int])
E        +    where <bound method TypeParser.matches_type of <class 'pydra.utils.typing.TypeParser'>> = TypeParser.matches_type

/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/pydra/utils/tests/test_typing.py:513: AssertionError

This is a bit tricky as I can't quite remember what my desired logic was. However, I have decided the tests failed because they were too strict instead of a bug in the code. Basicially, it comes down to whether

tuple[int, ...] should match a target type of tuple[int] (in test_matches_type_tuple_ellipsis4) and likewise
list[int] be able to be coerced into tuple[float, float, float]

Given the philosophy of permissive typing at construction time, i.e. if it could be ok, don't raise an error just because it isn't guaranteed to be (error will be raised at runtime if it is a problem), I think both these cases are ok and therefore I have relaxed the tests accordingly

@djarecka djarecka merged commit 1858668 into nipype:master Feb 27, 2024
43 checks passed
@tclose tclose deleted the subclass-permissive-typing branch February 29, 2024 07:42
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.

4 participants