Convert AnalysisOptions to Pydantic (Issue #78)

[yongminkim0501]

Overview

This PR converts the 'AnalysisOptions' class to use Pydantic for more pythonic serialization, as suggested in #78

Changes

• Changed 'AnalysisOptions' from '@DataClass' to 'pydantic.BaseModel'
• Updated 'to_json()' to use '.dict()' for serialization (Pydantic v1 API)
• Simplified "from_json()' using Pydantic's dict unpacking ('**dict')
• Added "pydantic>=1.9.0, <1.10.0' to dependencies

Technical Issue

• Used Pydantic v1.9 due to typing-extensions version conflict with signal-processing-algorithms==1.3.5
• Using '.dict()' instead of '.model_dump()' (v1.9 doesn't have model_dump, which is v2 only)

Testing

• All 68 tests passing

Future work

• If this approach is approved, I plan to continue converting other classes with similar serialization patterns to Pydantic.

Related Issue

Part of #78

[henrikingo]

Hi @yongminkim0501 and welcome!

As the person who wrote most of those to_json() methods, I'm glad to see them go away!

I have just one small comment, if you can make that change, I'll merge this. And yes by all means, please continue this with remaining classes.

[henrikingo]

Until the next release, let's try to keep some superficial backward compatibility. (After that we will make big changes anyway, so then this can be removed.)

Could you keep all to_json() functions and make them do something like:

@deprecated
def to_json(self):
    return self.dict()

These functions are called by outside code that depends on otava, so this could ease the pain if upgrading to the next release. (At the same time, note that we don't promise and don't test for strict backward combatibility at this stage of the projects lifecycle.

[yongminkim0501]

Thanks for the feedback! I'll update the PR to deprecate the to_json() methods
instead of removing them. Will commit the changes shortly.

[yongminkim0501]

I’ve applied the requested changes and pushed a possible fix for the suspected error. I’d appreciate it if you could take a look.

[henrikingo]

This question was in my emails, although it seem to have disappeared from github...

I have a question about the scope of this PR. Converting classes to Pydantic BaseModel requires changing the initialization syntax from positional arguments to keyword arguments.

'''python

Before
Metric(1, 1.0)

After (Pydantic)
Metric(direction=1, scale=1.0)
'''

This affects:

Test code (which I can update)
External users who depend on otava
I want to make sure I'm following the intended migration path. Thanks!

Hmm... Good catch ...

Ok so testing and googling a little myself, I learn that if we want to preserve backward compatibility, maybe you should look into https://docs.pydantic.dev/1.10/usage/dataclasses/

Now that 0.7.0 release is already in the owen... Let me also repeat that if we just wait past that release, there's no need to maintain any backward compatibility. Instead, we can discuss about what is our preferred way to maintain these data models? For example, my personal view would be:

• The main goal of this ticket is to get rid of the to_json() and from_json() methods and replace them with some standard utility class.
• As of keeping Metric(1,1.0) working, I'm lazy, so less typing is welcome. Even for yourself, you might find that maintaining backward compatibility is less work that rewriting all the tests.
• Speaking purely subjectively, I'm not a big fan of creating any classes for data that could have been just a dict. (The dict could of course be wrapped in a class that provides type information and get/set accessors.) But I assume this is a minority view so I'm not pushing it strongly.

I'll now look into your other questions and re-review what changed.

[henrikingo]

Ok so nothing inherently wrong in the patch and no other comments for now. But I suggest you look into pydantic.dataclass next, and if that works and helps us get rid of the to_json()/from_json() functions, then even this very patch will become simpler, as tests should just continue to work as they are.

[yongminkim0501]

Thanks for the suggestion! I'll use pydantic.dataclass to make the tests pass while maintaining backward compatibility.

Looking forward to discussing the preferred approach for data model management after the 0.7.0 release!