Skip to content

Rewrite MPS parsing to use dataclasses instead of dicts #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

Merged
merged 7 commits into from
Apr 16, 2025

Conversation

MBradbury
Copy link
Contributor

In the spirit of breaking out changes from #807, this PR includes rewriting parsing MPS files to use dataclasses instead of dicts.

This change does not add any new capabilities, and was done to:

  1. Make it easier to type check the MPS parsing
  2. To produce a more structured output from MPS parsing

As raised in #807, a downside is that this adds a dependency on dacite, whereas there were not previously any (depending on which backend is in use).

@pchtsp
Copy link
Collaborator

pchtsp commented Apr 10, 2025

how to you feel about using TypedDicts instead of dataclasses? This way we keep dictionaries and still have type checking without needing extra dependencies.

@MBradbury
Copy link
Contributor Author

I think dataclasses are the better approach.

Three other options:

  1. Do not support converting from a dict back to an LpProblem (expect users to go via an MPS file)
  2. Expect users to pickle the dataclasses (not human-readable)
  3. Roll something equivalent to dacite.from_dict (All dacite does is provide this function, so not sure this would be sensible)

@pchtsp
Copy link
Collaborator

pchtsp commented Apr 10, 2025

Understood. Let's just add the dacite dependency as you propose. Thanks,

@MBradbury
Copy link
Contributor Author

MBradbury commented Apr 10, 2025

I'm taking a look at how much extra code it might be to implement converting dicts to a dataclass:

@dataclass
class MPSParameters:
    name: str
    sense: int
    status: int
    sol_status: int

    @classmethod
    def fromDict(cls, data: dict[str, Any]) -> MPSParameters:
        return cls(data["name"], data["sense"], data["status"], data["sol_status"])

Each of the MPS dataclasses will just need to manually parse the dict.

Copy link
Collaborator

@pchtsp pchtsp 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 this, it's looking very good. I see that you added some from_dict and fromDict to cover the previous methods in some cases. But I think that many classes have lost them. Can you check and add the (now useless) fromDict, from_dict, toDict and to_dict. Ideally, with some kind of deprecation warning? So that we can take them out in a next version.

For example, LpVariable is missing from_dict and fromDict methods. LpProblem is missing to_dict and from_dict.

Others are missing more.

I hate to being picky about this but I already had issues removing an undocumented alias I had for a method and someone, somewhere came back to complain I did not add a deprecation warning before removing it.

@classmethod
def fromDict(cls, dj=None, varValue=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're missing the fromDict methods somewhere. Just for deprecation reasons, even if they're not useful anymore in pulp.

@MBradbury
Copy link
Contributor Author

Do you want all fromDict / toDict methods marked as deprecated or just from_dict / to_dict?

@pchtsp
Copy link
Collaborator

pchtsp commented Apr 15, 2025

I think the fromDict and toDict should stay without the deprecation warning as I would see them being part of the API, even if not very well documented.

Thanks again.
Also, feel free to add your name to the AUTHORS file as I see you're contributing regularly.

Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

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

a typo in a warning. Besides that I think it's ready to merge. Right?

pulp/pulp.py Outdated
:rtype: list
"""
warnings.warn(
"LpAffineExpression.to_dict is deprecated, use LpAffineExpression.toDataclass instead",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here it should say: "... use LpAffineExpression.toDict instead"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@MBradbury
Copy link
Contributor Author

Yes, I think all good to merge.

@pchtsp pchtsp merged commit d4763a8 into coin-or:master Apr 16, 2025
14 checks passed
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