-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix alternative platform loading #1151
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
=======================================
Coverage 52.60% 52.60%
=======================================
Files 71 71
Lines 3173 3173
=======================================
Hits 1669 1669
Misses 1504 1504
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Unfortunately, this part is harder to test, since a platform is required. But I could try to create a dummy-based one, to attempt reproducing the issue. |
Ok, the recursive dumping fully prevents to have actual objects, since they are not loaded back. So, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alecandido.
Unfortunately, this part is harder to test, since a platform is required. But I could try to create a dummy-based one, to attempt reproducing the issue.
I also tried to reproduce the issue to make sure I understand it correctly and I ended up adding a test in #1153. In any case, we can merge this now since it is a needed fix and discuss the test in the other PR.
The loading mechanism provided in #1123 is convenient for the future. But, unfortunately, to implement it as intended, we should make some more magic to make it work.
Indeed, since we rely on Pydantic for (de)serialization, we need to be pretty specific with the possible types.
However, this is not the case for the
InstrumentMap
, where we are specifying the parent class, rather than the union of its children.We already faced this problem for configuration, leading to
ConfigKinds
, which is not the simplest object.For the time being, I decided to bypass the problem, since we do not have any actual need to serialize the
Hardware
object, because our persistence layer for hardware just consists of theplatform.py
files (which we are reading, but aiming to write automatically).