Skip to content

Conversation

@luxaritas
Copy link
Contributor

This brings parity with the Java version (I also found that DeepPlanningClone did not work on a planning solution ProblemFactCollectionProperty, whereas marking the entire class to deep clone does)

@luxaritas luxaritas changed the title Support deep_planning_clone class decorator feat: Support deep_planning_clone class decorator Dec 11, 2024
@luxaritas luxaritas force-pushed the deep-planning-clone-class branch from bef4b6a to e13b83c Compare December 11, 2024 04:35
@triceo
Copy link
Collaborator

triceo commented Dec 11, 2024

Thank you for your contribution, @luxaritas!

We are a bit short-handed at the moment, with many people already taking their year-end holidays. I'm going to leave this one open until my colleague, who owns the Python part of the codebase, comes back in early January and does a code review.

Please be patient - we have every intention to merge this PR.

@triceo triceo added this to the v1.18.0 milestone Dec 11, 2024
@triceo
Copy link
Collaborator

triceo commented Dec 11, 2024

On a general note though:

  • Please make sure the existing test coverage passes. At the moment, it does not:

    py310: commands[0]> pytest --import-mode=importlib python/python-core/tests
    ImportError while loading conftest '/Users/runner/work/timefold-solver/timefold-solver/python/python-core/tests/conftest.py'.
    python/python-core/tests/conftest.py:1: in
    import timefold.solver
    .tox/py310/lib/python3.10/site-packages/timefold/solver/init.py:50: in
    import timefold.solver.domain as domain
    .tox/py310/lib/python3.10/site-packages/timefold/solver/domain/init.py:28: in
    from ._annotations import *
    .tox/py310/lib/python3.10/site-packages/timefold/solver/domain/_annotations.py:876: in
    def deep_planning_clone(entity_class: Type[A] = None) -> Type[A]: E NameError: name 'A' is not defined
    py310: exit 4 (0.21 seconds) /Users/runner/work/timefold-solver/timefold-solver> pytest --import-mode=importlib python/python-core/tests pid=3876

This issue also happens on Python 3.11 and 3.12. (We do not yet support 3.13.)

  • Please introduce extra coverage for your change. There ought to be a test to check whether the deep-cloning works when declared.

@luxaritas
Copy link
Contributor Author

Completely understand folks being out - I have my own local adjustments for my PRs so no big deal for me anyways!

I caught the CI failure after I'd already stepped away - copy-paste error. Will look into tests as well.

Copy link
Contributor

@Christopher-Chianelli Christopher-Chianelli left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
Docstring for attribute DeepPlanningClone should still mention planning_entity/planning_solution (those attributes are automatically planning cloned). Also need to test if a new instance is created in the test.
The actual implementation looks correct.

@triceo
Copy link
Collaborator

triceo commented Jan 7, 2025

Hello @luxaritas - 2025 is here, we are back, and your PR's been reviewed.
Please resolve the comments and the one minor conflict, so that we can merge your work.

@luxaritas
Copy link
Contributor Author

Resolved and rebased!

@luxaritas luxaritas had a problem deploying to documentation (preview) January 9, 2025 05:48 — with GitHub Actions Failure
@triceo triceo requested review from Christopher-Chianelli and removed request for Christopher-Chianelli January 9, 2025 05:49
@triceo triceo dismissed Christopher-Chianelli’s stale review January 9, 2025 05:52

The requested changes were made.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2025

@triceo triceo merged commit 4316a5e into TimefoldAI:main Jan 9, 2025
9 of 10 checks passed
@triceo
Copy link
Collaborator

triceo commented Jan 9, 2025

Thank you, @luxaritas! Scheduled for a next-week release.

@luxaritas luxaritas deleted the deep-planning-clone-class branch January 9, 2025 13:44
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.

3 participants