Skip to content

fix: Use Python object for Score, handle descriptor correctly, do not copy parent fields #66

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

Conversation

Christopher-Chianelli
Copy link
Collaborator

  • Using Python object for Score allows Python libraries to interpret the
    score, allowing the @planning_solution class, justification and
    other Python classes that has a score to be used by Pydantic and
    the like. This also allows Score | None to be used in type
    annotations.

  • Handle descriptors correctly. Previously, instance fields were
    generated for them (which is incorrect, since they are functions
    to be called by __getattribute__). Now, the static attributes
    of a Python class are checked, and all static attributes that
    are descriptors are removed from the instance field candidate
    set. This uses an API that was added in Python 3.11
    (inspect.getmembers_static). For Python 3.10, we copied the
    inspect.getmembers_static code.

  • Previously, parent fields were sometimes added to the instance
    field candidate set, causing some fields to incorrectly be None
    when unwrapping a PythonLikeObject. Now, all parent fields are
    removed from the instance field candidate set.

  • Made toString() call $method$__str__(), and change the return
    type of $method$__str__() to PythonString so overrides work
    correctly.

… copy parent fields

- Using Python object for Score allows Python libraries to interpret the
  score, allowing the @planning_solution class, justification and
  other Python classes that has a score to be used by Pydantic and
  the like. This also allows `Score | None` to be used in type
  annotations.

- Handle descriptors correctly. Previously, instance fields were
  generated for them (which is incorrect, since they are functions
  to be called by __getattribute__). Now, the static attributes
  of a Python class are checked, and all static attributes that
  are descriptors are removed from the instance field candidate
  set. This uses an API that was added in Python 3.11
  (inspect.getmembers_static). We do a best attempt in Python 3.10
  to resolve descriptor correctly. This causes test failures for
  Python 3.10 in jpyinterpreter but not for timefold.solver. Tests
  in Python 3.11 and above all pass.

- Previously, parent fields were sometimes added to the instance
  field candidate set, causing some fields to incorrectly be None
  when unwrapping a PythonLikeObject. Now, all parent fields are
  removed from the instance field candidate set.

- Made toString() call $method$__str__(), and change the return
  type of $method$__str__() to PythonString so overrides work
  correctly.
- Note: Python Software Foundation License is similar to
  the Apache license; https://www.apache.org/legal/resolved.html#category-a
Copy link

sonarqubecloud bot commented Jun 6, 2024

@zepfred
Copy link
Contributor

zepfred commented Jun 7, 2024

Using Python object for Score allows Python libraries to interpret the
score, allowing the @planning_solution class, justification and
other Python classes that has a score to be used by Pydantic and
the like. This also allows Score | None to be used in type
annotations.

There are some nice Java tests validating the conversion between Python and Java objects. However, I couldn't find any Python code using or testing the new score structure. I recommend adding some domain/tests using medium and bendable scores.

Previously, parent fields were sometimes added to the instance
field candidate set, causing some fields to incorrectly be None
when unwrapping a PythonLikeObject. Now, all parent fields are
removed from the instance field candidate set.

There are some new tests, but I'm not entirely sure they cover all the behavior added by this change. Am I wrong?

Made toString() call $method$__str__(), and change the return
type of $method$__str__() to PythonString so overrides work
correctly.

I noticed that you didn't update all Python types, such as 'PythonLikeList'. Is there any specific reason for that? If not, please double-check to ensure all the required types are updated.

@Christopher-Chianelli
Copy link
Collaborator Author

Using Python object for Score allows Python libraries to interpret the
score, allowing the @planning_solution class, justification and
other Python classes that has a score to be used by Pydantic and
the like. This also allows Score | None to be used in type
annotations.

There are some nice Java tests validating the conversion between Python and Java objects. However, I couldn't find any Python code using or testing the new score structure. I recommend adding some domain/tests using medium and bendable scores.

That because all the existing tests (which previously uses the Java score) uses the Python score and weren't change

Previously, parent fields were sometimes added to the instance
field candidate set, causing some fields to incorrectly be None
when unwrapping a PythonLikeObject. Now, all parent fields are
removed from the instance field candidate set.

There are some new tests, but I'm not entirely sure they cover all the behavior added by this change. Am I wrong?

The Score python classes cover this behaviour.

Made toString() call $method$__str__(), and change the return
type of $method$__str__() to PythonString so overrides work
correctly.

I noticed that you didn't update all Python types, such as 'PythonLikeList'. Is there any specific reason for that? If not, please double-check to ensure all the required types are updated.

Generally speaking, only the Python type that overridden toString would need to be updated.

@zepfred zepfred self-requested a review June 10, 2024 12:20
@zepfred
Copy link
Contributor

zepfred commented Jun 10, 2024

Using Python object for Score allows Python libraries to interpret the
score, allowing the @planning_solution class, justification and
other Python classes that has a score to be used by Pydantic and
the like. This also allows Score | None to be used in type
annotations.

There are some nice Java tests validating the conversion between Python and Java objects. However, I couldn't find any Python code using or testing the new score structure. I recommend adding some domain/tests using medium and bendable scores.

That because all the existing tests (which previously uses the Java score) uses the Python score and weren't change

Okay, I still believe that creating Python classes using the new structure is a good idea, similar to what we did in tests/test_domain.py. Regardless, this is not a blocker if you ensured testing the new score classes locally.

Made toString() call $method$__str__(), and change the return
type of $method$__str__() to PythonString so overrides work
correctly.

I noticed that you didn't update all Python types, such as 'PythonLikeList'. Is there any specific reason for that? If not, please double-check to ensure all the required types are updated.

Generally speaking, only the Python type that overridden toString would need to be updated.

That means the non-updated types will continue using the "incorrect" logic. In this case, we should keep the new logic consistent across all types.

@Christopher-Chianelli
Copy link
Collaborator Author

That means the non-updated types will continue using the "incorrect" logic. In this case, we should keep the new logic consistent across all types.

No, the logic for things without toString would be the same.

Copy link
Contributor

@zepfred zepfred left a comment

Choose a reason for hiding this comment

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

The new score classes and the updated internal behavior are significant improvements!

@Christopher-Chianelli Christopher-Chianelli merged commit dc5fc78 into TimefoldAI:main Jun 10, 2024
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