Skip to content

feat: port meeting-scheduling from Java to Python #831

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

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

blackopsrepl
Copy link

@blackopsrepl blackopsrepl commented Jun 17, 2025

Description of the change

This is a follow-up to the code review for PR #824 by @Christopher-Chianelli :

  • Aligned init.py to the other quickstarts
  • Refactored demo-data generation to use random.Random and a CountDistribution dataclass
  • Replaced SolutionManager with ConstraintVerifier in tests
  • Removed meeting_scheduling.egg-info
  • Unified solver and API into one pydantic data model

Checklist

Development

[✅] The changes have been covered with tests, if necessary.
[✅] You have a green build, with the exception of the flaky tests
[✅] UI and JS files are fully tested, the user interface works for all modules affected by your changes (e.g., solve and analyze buttons).
[✅] The network calls work for all modules affected by your changes (e.g., solving a problem).
[✅] The console messages are validated for all modules affected by your changes.

Note:
The Python port stops solving with an approximate score of Score: 0hard/~50medium/~4500soft against 0hard/0medium/~3000soft of the original Java quickstart.

Code Review

  • This pull request includes an explanatory title and description.
  • The GitHub issue is linked.
  • At least one other engineer has approved the changes.
  • After PR is merged, inform the reporter.

- Aligned init.py to the other quickstarts
- Refactored demo-data generation to use random.Random and a CountDistribution dataclass
- Replaced SolutionManager with ConstraintVerifier in tests
- Removed meeting_scheduling.egg-info
- Misc. clean-up
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.

Fantastic work! Beside unifying the Solver and Pydantic models, a few review comments. If you need help or encounter any issues, let me know!

@blackopsrepl blackopsrepl force-pushed the python-ports/meeting-scheduling branch from 0c6fbb8 to c769b83 Compare June 21, 2025 20:20
-Migrated to unified pydantic model
-Restored Attendance class
-Implemented global constants and helper functions in test_constraints to avoid repetition
-Removed overlapping tests
-Added test_feasible
@blackopsrepl blackopsrepl force-pushed the python-ports/meeting-scheduling branch from c769b83 to f8dc033 Compare June 21, 2025 20:23
@blackopsrepl
Copy link
Author

Hello @Christopher-Chianelli, thanks for your answers. I think I have covered everything, but as stated in the final PR, scores from the Python port are worse than those from the original version.

@blackopsrepl blackopsrepl marked this pull request as ready for review June 21, 2025 20:26
@Christopher-Chianelli
Copy link
Contributor

Christopher-Chianelli commented Jun 23, 2025

@blackopsrepl I can confirm a 75% performance regression (i.e. the move evaluation speed is 25% of the original Python port). Pydantic does add some overhead unfortunately. That being said, the move evaluation speed is still acceptable.

Do note the input dataset generated by the Java version is slightly different then the input dataset generated by the Python version. This is expected; Java and Python have different random number generation implementations. This means the solutions cannot be directly compared unless you submit the Java's dataset to Python using the curl API. It appears the Java port and Python port expects different input JSON with the current code; I will look into that.

A Python port is expected to be slower than the original Java code; the Python code is translated to be one-to-one with CPython. This means we inherit many of CPython particularities (such as all integers being BigInts, needing to handling all kinds of ways of calling a method, and reading an attribute might be an overly complex operation). Being accurate is more important than speed; it is better to get the correct answer slower than an incorrect answer faster.

@blackopsrepl
Copy link
Author

@blackopsrepl I can confirm a 75% performance regression (i.e. the move evaluation speed is 25% of the original Python port). Pydantic does add some overhead unfortunately. That being said, the move evaluation speed is still acceptable.

Do note the input dataset generated by the Java version is slightly different then the input dataset generated by the Python version. This is expected; Java and Python have different random number generation implementations. This means the solutions cannot be directly compared unless you submit the Java's dataset to Python using the curl API. It appears the Java port and Python port expects different input JSON with the current code; I will look into that.

A Python port is expected to be slower than the original Java code; the Python code is translated to be one-to-one with CPython. This means we inherit many of CPython particularities (such as all integers being BigInts, needing to handling all kinds of ways of calling a method, and reading an attribute might be an overly complex operation). Being accurate is more important than speed; it is better to get the correct answer slower than an incorrect answer faster.

@Christopher-Chianelli Thank you for the details. To move forward:

  1. JSON Compatibility Test

    • I'll generate test fixtures from Java today
    • Will verify Python solver behavior against:
      • Java-generated input
      • Python-generated input
    • This will conclusively show if scores differ due to:
      • Python solver behavior (needs fixing)
      • RNG variance (can document)
  2. Merge Criteria

    • If scores match → Ready to merge
    • If not → I'll debug constraints
  3. Your Input Needed

    • Are you already working on JSON mismatch?
    • Any specific test cases to include?

The performance impact is understood and accepted. Hence I propose we resolve the scoring question empirically.

@Christopher-Chianelli
Copy link
Contributor

@Christopher-Chianelli Thank you for the details. To move forward:

1. **JSON Compatibility Test**
   
   * I'll generate test fixtures from Java today
   * Will verify Python solver behavior against:
     
     * Java-generated input
     * Python-generated input
   * This will conclusively show if scores differ due to:
     • Python solver behavior (needs fixing)
     • RNG variance (can document)

2. **Merge Criteria**
   
   * If scores match → Ready to merge
   * If not → I'll debug constraints

Due to the difference in score calculation speed, the Python Solver might not arrive at the same solution as the Java Solver (and thus have different score). You could compare the score analysis of the final Java Solver's solution using the "/schedules/analyze" endpoint.

3. **Your Input Needed**
   
   * Are you already working on JSON mismatch?

Currently working on something else; but I expect one problem is Meeting.speakers and Meeting.content; they are nullable/optional in the Java version, but not in the Pydantic model. There might be other issues though.

   * Any specific test cases to include?

All the test cases in https://github.com/TimefoldAI/timefold-quickstarts/blob/stable/java/meeting-scheduling/src/test/java/org/acme/meetscheduling/solver/MeetingSchedulingConstraintProviderTest.java (you might already have them).

@blackopsrepl blackopsrepl force-pushed the python-ports/meeting-scheduling branch from cc6fc9b to f8dc033 Compare June 26, 2025 08:45
Changed from method calls (hard_score()) to property access (hard_score)
for score components in the status response.
(cherry picked from commit 407cb879bb797d9d51f85dad9c2fa0ff2c0819dc)
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