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

DM-38499: Fix minor ruff issue and re-enable tests #18

Merged
merged 6 commits into from
Jul 14, 2023
Merged

Conversation

timj
Copy link
Member

@timj timj commented Jul 11, 2023

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -29.95 ⚠️

Comparison is base (db5f6d9) 69.23% compared to head (bb9296b) 39.28%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #18       +/-   ##
===========================================
- Coverage   69.23%   39.28%   -29.95%     
===========================================
  Files           3       10        +7     
  Lines          26      336      +310     
  Branches        6       60       +54     
===========================================
+ Hits           18      132      +114     
- Misses          6      198      +192     
- Partials        2        6        +4     
Impacted Files Coverage Δ
tests/test_import.py 100.00% <ø> (+25.00%) ⬆️
tests/test_config.py 100.00% <100.00%> (+50.00%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@timj timj requested a review from PaulPrice July 11, 2023 23:35
@timj
Copy link
Member Author

timj commented Jul 11, 2023

The tests don't work at all with rubin-env 6 because, at least on my macOS system, parsl 1.2.0 won't import at all:

>>> import parsl
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/__init__.py", line 22, in <module>
    from parsl.app.app import bash_app, join_app, python_app
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/app/app.py", line 12, in <module>
    from parsl.dataflow.dflow import DataFlowKernel
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/dataflow/dflow.py", line 42, in <module>
    class DataFlowKernel(object):
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/dataflow/dflow.py", line 61, in DataFlowKernel
    def __init__(self, config=Config()):
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/config.py", line 88, in __init__
    executors = [ThreadPoolExecutor()]
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/parsl/executors/threads.py", line 32, in __init__
    def __init__(self, label: str = 'threads', max_threads: int = 2,
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/typeguard/_functions.py", line 135, in check_argument_types
    check_type_internal(value, annotation, memo)
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/typeguard/_checkers.py", line 756, in check_type_internal
    checker(value, origin_type, args, memo)
  File "/Users/timj/work/lsstsw/miniconda/envs/lsst-scipipe-6.0.0/lib/python3.10/site-packages/typeguard/_checkers.py", line 284, in check_list
    raise TypeCheckError("is not a list")
typeguard.TypeCheckError: argument "storage_access" (None) is not a list

(the check is correct since the parameter is declared as storage_access: List[Any] = None)

@benclifford
Copy link

That optional-violation line was fixed here: Parsl/parsl#2509

For context: It used to be that = None marked a type as implicitly optional, but that changed in python/peps#689 in 2018 and I guess typeguard was still being liberal in that way at some point it wasn't.

It looks like this changed in typeguard earlier this year with a major version change to 3, which Parsl 1.2.0 has no upper bound for. More recent Parsl releases have an upper bound of <3, which is incompatible with what I've seen installed in lsst_distrib but as far as I can see the current Parsl codebase works fine with the current typeguard 4.0.0 so I think that constraint will get loosen soon.

@timj
Copy link
Member Author

timj commented Jul 12, 2023

Yes, I remember having to fix the annotations in butler. I'm a bit confused as to how anyone is using parsl with Rubin env 6 though. We are imminently releasing 7 with newer parsl so I won't merge until that comes out.

@benclifford
Copy link

I'm a bit confused as to how anyone is using parsl with Rubin env 6 though.

I think this broke on March 14, 2023 with the release of typeguard 3, so I think anyone with a base environment older than that wouldn't hit this problem. Plenty of DESC-related people also use the desc branch rather than parsl 1.2 from the base environment (that's the latest parsl plus my DESC-relevant prototyping/hacks that aren't release-worthy)

@timj timj merged commit fe186b8 into main Jul 14, 2023
@timj timj deleted the tickets/DM-38499 branch July 14, 2023 03:49
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