remove 'decorator' dependency #1513

danieleades · 2024-02-11T09:51:24Z

besides removing an unneeded dependency (always good), using a 'native' decorator function allows for creating a fully-typed decorator

decorated methods are now fully type annotated
unblocks enabling stricter mypy configuration
helps solve the errors in add 'flake-future-annotations' lint group #1506...

sisp

It looks like you ran poetry lock after you had removed the decorator/types-decorator dependencies from pyproject.toml, which resulted in a full relock of the entire dependency tree. To avoid mixing concerns, could you please restore the old version of poetry lock and run poetry lock --no-update instead? The changes to poetry.lock should be limited to removing the decorator-related dependencies then.

danieleades · 2024-02-11T10:56:08Z

@sisp changes to lockfile reverted

sisp

I have a few inline questions.

Do you happen to know why some tests are failing now? Off the top of my head, I'm not sure why these changes would fail those tests. 🤔

sisp · 2024-02-11T11:18:56Z

poetry.lock

-    {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"},
-    {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"},
-    {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"},
-    {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"},
-    {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"},
-    {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"},
-    {file = "PyYAML-6.0.1-cp312-cp312-win_amd64.whl", hash = "sha256:0d3304d8c0adc42be59c5f8a4d9e3d7379e6955ad754aa9d6ab7a398b59dd1df"},


Which version of Poetry are you using? I wonder why, e.g., these locked versions have been removed. 🤔 Specifically here, all Python 3.12 wheel files have been removed; this can't be good.

i did think that was a touch odd

poetry --version Poetry (version 1.7.1)

Odd. Could you try running poetry update pexpect pyyaml and check whether those deleted lines get re-added? Might be a Poetry bug ...

poetry update pexpect pyyaml

no effect

copier/cli.py

danieleades · 2024-02-11T11:35:23Z

I have a few inline questions.

Do you happen to know why some tests are failing now? Off the top of my head, I'm not sure why these changes would fail those tests. 🤔

~~i'm very confused about the test failures, because i don't see those locally...~~

Ah i think i see it. Plumbum is relying on some introspection of the arguments that functools.wraps is not preserving. Without the functools.wraps decorator a whole bunch more tests fail.

It seems that Plumbum does not play nicely with decorators. the decorator library works harder to preserve the wrapped functions signature so it works with Plumbum, but decorator doesn't support type annotations.

Doesn't seem like you get all of this to work at once using Plumbum.

I'll spend a tiny bit more time on this and see if i can find a resolution

danieleades · 2024-02-11T12:30:51Z

so this turned out to be a bit of a rabbit hole.

I can fix the issue with plumbum easily enough by adding inner.__signature__ = inspect.signature(method):

def _handle_exceptions(method: Callable[P, int]) -> Callable[P, int]:
    def inner(*args: P.args, **kwargs: P.kwargs) -> int:
        try:
            try:
                return method(*args, **kwargs)
            except KeyboardInterrupt:
                raise UserMessageError("Execution stopped by user")
        except UserMessageError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            return 1
        except UnsafeTemplateError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            # DOCS https://github.com/copier-org/copier/issues/1328#issuecomment-1723214165
            return 0b100

    # See https://github.com/copier-org/copier/pull/1513
    inner.__signature__ = inspect.signature(method)  # type: ignore[attr-defined]

    return inner

that works perfectly

until i add from __future__ import annotations...

Adding that changes the behaviour of the inspect.signature method - https://bugs.python.org/issue43355, which is probably what was causing the issue in #1506 in the first place.
I can resolve that by adding eval_str=True to inspect.signature, but that flag is only supported in python 3.10+

danieleades · 2024-02-11T12:32:34Z

@sisp would you kindly trigger the CI for me?

or better yet, merge the fairly trivial #1505 so that CI will auto-trigger for my PRs?

sisp · 2024-02-11T13:06:46Z

Thanks for tracing this problem, @danieleades! 🙇

So IIUC, with and without the decorator library is from __future__ import annotations a problem because it requires inspect.signature's eval_str=True, which isn't available before Python 3.10. Correct?

So, #1506 is blocked in any case as long as we need to support Python 3.8 & 3.9, which we need to keep supporting until EOL. This means, we can't use a decorator if we want #1506. Right? Perhaps we could replace the decorator by a context manager?

def main(self, ...):
    with _handle_exceptions():
        ...

It would be less elegant but should solve the problem unless I'm missing something. WDYT?

codecov · 2024-02-11T14:04:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (74bba54) 97.33% compared to head (84310d5) 97.27%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1513      +/-   ##
==========================================
- Coverage   97.33%   97.27%   -0.07%     
==========================================
  Files          48       48              
  Lines        4537     4543       +6     
==========================================
+ Hits         4416     4419       +3     
- Misses        121      124       +3

Flag	Coverage Δ
unittests	`97.27% <100.00%> (-0.07%)`	⬇️

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.

danieleades · 2024-02-11T14:43:11Z

Thanks for tracing this problem, @danieleades! 🙇

So IIUC, with and without the decorator library is from __future__ import annotations a problem because it requires inspect.signature's eval_str=True, which isn't available before Python 3.10. Correct?

So, #1506 is blocked in any case as long as we need to support Python 3.8 & 3.9, which we need to keep supporting until EOL. This means, we can't use a decorator if we want #1506. Right? Perhaps we could replace the decorator by a context manager?
def main(self, ...):
    with _handle_exceptions():
        ...
It would be less elegant but should solve the problem unless I'm missing something. WDYT?

i think a context manager would be less 'magic' to be honest, so might be worth considering

I think this can be made to work as is, just that from __future__ import annotations can't be added to this specific file until after dropping support for python 3.9. Or maybe it's possible to upstream a fix into plumbum.cli?

it's less clear now that this PR adds value, but i personally would argue its still a marginal improvement

sisp · 2024-02-11T15:16:18Z

If an upstream fix in plumbum.cli would help, that might be a good way forward. Omitting from __future__ import annotations from this file might be okay, too, although I tend to prefer consistency.

I'm a bit undecided about the best way forward (also regarding unblocking #1506). Any opinions, @copier-org/maintainers?

Drop decorator/types-decorator and use a context manager for handling exceptions.
Omit from __future__ import annotations from this file and wait until Python 3.9 reaches EOL.
Attempt to find an upstream fix in plumbum.cli to support decorators with from __future__ import annotations without requiring inspect.signature's eval_str argument.
Other ideas?

danieleades · 2024-02-11T17:14:28Z

4. Other ideas?

just using a plain method to handle errors seems pretty clean to me. Have updated the PR to use this approach

danieleades · 2024-02-12T07:45:00Z

If an upstream fix in plumbum.cli would help, that might be a good way forward

i did open an upstream issue, but to be honest i think sidestepping the decorator approach entirely is probably the way forward here. It's easier to reason about, and is actually less lines of code

pawamoy · 2024-02-12T08:17:17Z

I agree with @sisp: the worker context manager will let exceptions bubble up on exit, and the function call approach does not catch those. So I'd recommend using the context manager approach for handling exceptions instead.

sisp · 2024-02-12T08:19:59Z

@pawamoy Do you happen to have an idea why some locked pyyaml wheel files for Python 3.12 are removed from poetry.lock by Poetry when running poetry lock --no-update?

pawamoy · 2024-02-12T08:32:57Z

@sisp I don't! I stopped using Poetry more than a year ago 🙂
The removed entries are from other systems. Looks like the lock file stops being cross-platform or something? Dependabot might do something differently 🤷

If I restore the lock file from master branch into this branch, then run poetry lock --no-update again, it doesn't remove the entries. Here's the diff:

poetry.lock --- 1/3 --- TOML
238 238 [package.extras]
239 239 toml = ["tomli"]
240 ... 
241 ... [[package]]
242 ... name = "decorator"
243 ... version = "5.1.1"
244 ... description = "Decorators for Humans"
245 ... optional = false
246 ... python-versions = ">=3.5"
247 ... files = [
248 ...     {file = "decorator-5.1.1-py3-none-any.whl", hash = "sha256:b8c3f85900b9dc423225913c5aace94729fe1fa9763b38939a95226f02d37186"},
249 ...     {file = "decorator-5.1.1.tar.gz", hash = "sha256:637996211036b6385ef91435e4fae22989472f9d571faba8927ba8253acbc330"},
250 ... ]
251 240 
252 241 [[package]]
253 242 name = "distlib"

poetry.lock --- 2/3 --- TOML
1503 1492     {file = "types_backports-0.1.3-py2.py3-none-any.whl", hash = "sha256:dafcd61848081503e738a7768872d1dd6c018401b4d2a1cfb608ea87ec9864b9"},
1504 1493 ]
1505 .... 
1506 .... [[package]]
1507 .... name = "types-decorator"
1508 .... version = "5.1.8.20240106"
1509 .... description = "Typing stubs for decorator"
1510 .... optional = false
1511 .... python-versions = ">=3.8"
1512 .... files = [
1513 ....     {file = "types-decorator-5.1.8.20240106.tar.gz", hash = "sha256:32ff92b33615060d23b9d3760124bdb3506c4aa8d9eb50963cf1a3c20b9ecbbf"},
1514 ....     {file = "types_decorator-5.1.8.20240106-py3-none-any.whl", hash = "sha256:14d21e6a0755dbb8f301f2f532b3eab5148f433c69dad2d98bf5bd2b3a2ef4e7"},
1515 .... ]
1516 1494 
1517 1495 [[package]]
1518 1496 name = "types-psutil"

poetry.lock --- 3/3 --- TOML
1652 [metadata]                                                                              1630 [metadata]
1653 lock-version = "2.0"                                                                    1631 lock-version = "2.0"
1654 python-versions = ">=3.8"                                                               1632 python-versions = ">=3.8"
1655 content-hash = "68ae53c03a93971c92d62fdd18a7635f2b9a7d2a42317b781777412020227e01"       1633 content-hash = "e77e027798e8d257d07fee377fb2581be217bbcd761326d043f3b5006e6e870f"

(by the way, not sure where this fancy git diff output comes from 😄 I must have installed something extra. EDIT: I thought it was diff-so-fancy, but it looks like it comes from git itself!)

sisp · 2024-02-12T08:38:45Z

Thanks, @pawamoy! 🙏

@danieleades, could you try restoring poetry.lock from master as @pawamoy described?

danieleades · 2024-02-12T08:54:59Z

Thanks, @pawamoy! 🙏

@danieleades, could you try restoring poetry.lock from master as @pawamoy described?

I have tried that, doesn't help.
Might be to do with my environment - I'm not using the Nix environment.

If I get the time tonight I'll have a play around with it

danieleades · 2024-02-12T19:36:34Z

I agree with @sisp: the worker context manager will let exceptions bubble up on exit, and the function call approach does not catch those. So I'd recommend using the context manager approach for handling exceptions instead.

i've used the function approach to also wrap the context manager. It's starting to get a tiny bit unwieldy.

it's not at all clear to me how you'd do this with a context manager, as a context manager doesn't allow you to either return a value or re-raise exceptions. What would a context manager based approach look like?

sisp · 2024-02-13T07:32:20Z

Reraising seems to be possible when the __exit__ method returns False: https://stackoverflow.com/a/18399069 But I had only thought about catching exceptions via the context manager but not about returning a corresponding CLI return code. 🤔

I think your current approach works. Just one more idea: Would keeping the original decorator approach by replacing the decorator library by wrapt work? wrapt doesn't seem to have type hints though. 🙁

danieleades · 2024-02-13T09:40:54Z

Reraising seems to be possible when the __exit__ method returns False

sort of. That doesn't give you the opportunity to change the type of the exception. the current code does this-

        try:
            return method(*args, **kwargs)
        except KeyboardInterrupt:
            raise UserMessageError("Execution stopped by user")

i can't think how you can both re-raise the UserMessageError and return False.

While i think of it, this code snippet should probably be:

        try:
            return method(*args, **kwargs)
        except KeyboardInterrupt as e:
            raise UserMessageError("Execution stopped by user") from e

danieleades · 2024-02-13T13:23:06Z

another approach that would work would be to go back to the decorator approach (without the decorator dependency) and add a second inner method directly in the class, like so-

def handle_exceptions(method: Callable[P, int]) -> Callable[P, int]:
    def inner(*args: P.args, **kwargs: P.kwargs) -> int:
        try:
            try:
                return method(*args, **kwargs)
            except KeyboardInterrupt:
                raise UserMessageError("Execution stopped by user")
        except UserMessageError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            return 1
        except UnsafeTemplateError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            # DOCS https://github.com/copier-org/copier/issues/1328#issuecomment-1723214165
            return 0b100

    return inner

# ...

    @handle_exceptions
    def _main(self, destination_path: cli.ExistingDirectory = ".") -> int:
        with self._worker(
            dst_path=destination_path,
            defaults=self.force or self.defaults,
            overwrite=self.force or self.overwrite,
            skip_answered=self.skip_answered,
        ) as worker:
            worker.run_recopy()
        return 0

    def main(self, destination_path: cli.ExistingDirectory = ".") -> int:
        """Call [run_recopy][copier.main.Worker.run_recopy].

        Parameters:
            destination_path:
                Only the destination path is needed to update, because the
                `src_path` comes from [the answers file][the-copier-answersyml-file].

                The subproject must exist. If not specified, the currently
                working directory is used.
        """
        return self._main(destination_path: cli.ExistingDirectory)

or, equivalently:

def handle_exceptions(method: Callable[P, None]) -> Callable[P, int]:
    def inner(*args: P.args, **kwargs: P.kwargs) -> int:
        try:
            try:
                method(*args, **kwargs)
                return 0
            except KeyboardInterrupt:
                raise UserMessageError("Execution stopped by user")
        except UserMessageError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            return 1
        except UnsafeTemplateError as error:
            print(colors.red | "\n".join(error.args), file=sys.stderr)
            # DOCS https://github.com/copier-org/copier/issues/1328#issuecomment-1723214165
            return 0b100

    return inner

# ...

    def main(self, destination_path: cli.ExistingDirectory = ".") -> int:
        """Call [run_recopy][copier.main.Worker.run_recopy].

        Parameters:
            destination_path:
                Only the destination path is needed to update, because the
                `src_path` comes from [the answers file][the-copier-answersyml-file].

                The subproject must exist. If not specified, the currently
                working directory is used.
        """

        @handle_exceptions
        def inner() -> int:
            with self._worker(
                dst_path=destination_path,
                defaults=self.force or self.defaults,
                overwrite=self.force or self.overwrite,
                skip_answered=self.skip_answered,
            ) as worker:
                worker.run_recopy()
        return inner()

sisp · 2024-02-13T15:04:10Z

Thanks for being persistent in trying to solve this problem, @danieleades! 🙏

This last idea has a bit too much duplicate code for my taste. In comparison, I'd prefer the inner() closure.

Have you tried wrapt instead of decorator for keeping the decorator-based approach? Does this work at runtime? And are the missing type hints okay in this case, as we don't call the decorated method ourselves? (I don't have access to a laptop right now. 🙁)

danieleades · 2024-02-13T15:29:41Z

Have you tried wrapt instead of decorator for keeping the decorator-based approach?

I haven't- though it does obviate the benefit of eliminating a dependency by introducing a new one

sisp · 2024-02-13T15:38:45Z

But perhaps a new one that works in combination with from __future__ import annotations unlike decorator?

sisp · 2024-02-13T15:44:43Z

I thought plumbum does work with from __future__ import annotations (tomerfiliba/plumbum#621), only our decorator breaks it. Or not?

danieleades · 2024-02-13T15:55:27Z

But perhaps a new one that works in combination with from __future__ import annotations unlike decorator?

it may well work, but it may not be necessary to introduce a dependency at all if it only saves 2-3 lines of code.
All else being equal, less dependencies is better

sisp · 2024-02-13T16:13:17Z

Okay, you convinced me. Let's keep the inner() closure approach and drop the decorator dependency. 👍 I'll be happy to merge the PR, just some minor cleanup left.

Could you please try to solve the poetry.lock issue and undo the formatting changes around DESCRIPTION_MORE which shouldn't be related to this PR? And I believe the __all__ dunder variable addition isn't strictly related to this PR, right? I think it makes sense to add them to all modules, but in a dedicated PR.

danieleades · 2024-02-19T10:15:03Z

Okay, you convinced me. Let's keep the inner() closure approach and drop the decorator dependency. 👍 I'll be happy to merge the PR, just some minor cleanup left.

Could you please try to solve the poetry.lock issue and undo the formatting changes around DESCRIPTION_MORE which shouldn't be related to this PR? And I believe the __all__ dunder variable addition isn't strictly related to this PR, right? I think it makes sense to add them to all modules, but in a dedicated PR.

i've updated as best i can, unfortunately i can't get the poetry lock behaviour to match the nix environment, and i'm having trouble setting up the nix environment because direnv is not playing nicely with my zsh shell.

Would you kindly update this PR by checking it out, updating the lockfile, and committing the change back to this branch?
That should be the last piece to get this merged!

pawamoy · 2024-02-19T10:37:34Z

If you look at this comment: #1513 (comment), you'll see that you can simply revert any changes to the lock file, and remove the two blocks marked with ... lines in the comment, as well as copy-paste the last block.

danieleades · 2024-02-19T10:43:32Z

If you look at this comment: #1513 (comment), you'll see that you can simply revert any changes to the lock file, and remove the two blocks marked with ... lines in the comment, as well as copy-paste the last block.

Are you sure? The lock file contains a hash. Do you know that those items are excluded from the hash?

pawamoy · 2024-02-19T11:01:25Z

Yep it was the right hash. I've pushed a commit anyway 😄
There's two more lines that have been removed to put back, I'll do that too.

danieleades · 2024-02-19T11:03:08Z

Yep it was the right hash. I've pushed a commit anyway 😄
There's two more lines that have been removed to put back, I'll do that too.

Cool, I wasn't sure what was included in that hash and have no way to compare!

pawamoy · 2024-02-19T11:03:58Z

OK done! The diff looks nice.

sisp

Perfect! 👌 Thanks for the detailed investigation and also final polishing! 🙏

danieleades force-pushed the remove-decorator branch from 69057c6 to fad3e75 Compare February 11, 2024 09:56

danieleades mentioned this pull request Feb 11, 2024

add 'flake-future-annotations' lint group #1506

Merged

sisp requested changes Feb 11, 2024

View reviewed changes

danieleades requested a review from sisp February 11, 2024 10:56

sisp enabled auto-merge (squash) February 11, 2024 11:05

sisp disabled auto-merge February 11, 2024 11:07

sisp enabled auto-merge (squash) February 11, 2024 11:07

sisp reviewed Feb 11, 2024

View reviewed changes

auto-merge was automatically disabled February 11, 2024 12:30
Head branch was pushed to by a user without write access

danieleades requested a review from sisp February 11, 2024 12:31

danieleades added 3 commits February 11, 2024 13:59

remove 'decorator' dependency

50574c6

fixup

52f1140

fixup

ad9b33e

danieleades force-pushed the remove-decorator branch from 091a1c4 to ad9b33e Compare February 11, 2024 14:00

danieleades added 3 commits February 11, 2024 14:05

fixup

fe86e4c

fixup

e5e0ef6

fixup

2877d73

danieleades added 2 commits February 11, 2024 17:10

use a plain method for handling errors

4010d97

revert formatting changes

a140afa

danieleades mentioned this pull request Feb 12, 2024

plumbum.cli doesn't play nicely with decorators and __future__.annotations tomerfiliba/plumbum#675

Open

handle errors in context handlers

87bb68f

revert changes

3c6ac64

chore: poetry lock --no-update

0ed3f08

chore: add back removed empty lines

fb0bb2b

chore: add back removed empty lines

84310d5

pawamoy approved these changes Feb 19, 2024

View reviewed changes

sisp approved these changes Feb 19, 2024

View reviewed changes

sisp merged commit 0924a75 into copier-org:master Feb 19, 2024
20 of 21 checks passed

danieleades deleted the remove-decorator branch February 19, 2024 13:56

remove 'decorator' dependency #1513

remove 'decorator' dependency #1513

Conversation

danieleades commented Feb 11, 2024 • edited Loading

sisp left a comment

Choose a reason for hiding this comment

danieleades commented Feb 11, 2024

sisp left a comment

Choose a reason for hiding this comment

sisp Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

danieleades Feb 11, 2024

Choose a reason for hiding this comment

sisp Feb 11, 2024

Choose a reason for hiding this comment

danieleades Feb 11, 2024

Choose a reason for hiding this comment

danieleades commented Feb 11, 2024 • edited Loading

danieleades commented Feb 11, 2024

danieleades commented Feb 11, 2024

sisp commented Feb 11, 2024

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

danieleades commented Feb 11, 2024

sisp commented Feb 11, 2024 • edited Loading

danieleades commented Feb 11, 2024

danieleades commented Feb 12, 2024

pawamoy commented Feb 12, 2024 • edited Loading

sisp commented Feb 12, 2024 • edited Loading

pawamoy commented Feb 12, 2024 • edited Loading

sisp commented Feb 12, 2024

danieleades commented Feb 12, 2024

danieleades commented Feb 12, 2024

sisp commented Feb 13, 2024

danieleades commented Feb 13, 2024

danieleades commented Feb 13, 2024 • edited Loading

sisp commented Feb 13, 2024

danieleades commented Feb 13, 2024

sisp commented Feb 13, 2024

sisp commented Feb 13, 2024

danieleades commented Feb 13, 2024

sisp commented Feb 13, 2024 • edited Loading

danieleades commented Feb 19, 2024

pawamoy commented Feb 19, 2024 • edited Loading

danieleades commented Feb 19, 2024

pawamoy commented Feb 19, 2024

danieleades commented Feb 19, 2024

pawamoy commented Feb 19, 2024

sisp left a comment

Choose a reason for hiding this comment

danieleades commented Feb 11, 2024 •

edited

Loading

sisp Feb 11, 2024 •

edited

Loading

danieleades commented Feb 11, 2024 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading

sisp commented Feb 11, 2024 •

edited

Loading

pawamoy commented Feb 12, 2024 •

edited

Loading

sisp commented Feb 12, 2024 •

edited

Loading

pawamoy commented Feb 12, 2024 •

edited

Loading

danieleades commented Feb 13, 2024 •

edited

Loading

sisp commented Feb 13, 2024 •

edited

Loading

pawamoy commented Feb 19, 2024 •

edited

Loading