Rework TransitionCriterion storage to remove circular dep #2320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
c4393a8
to
7dcbfa3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
7dcbfa3
to
0379199
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
0379199
to
ee768de
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
ee768de
to
40ac810
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
40ac810
to
746af25
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Apr 4, 2024
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
746af25
to
325c5bd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
) Summary: In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by: 1. Keeping all storage related logic in the storage files 2. Adding a specific check in object_from_json for ```TrialBasedCriterion``. These criterion contain lists of TrialStatuses, specifically in not_in_statuses and only_in_statuses args, which makes the standard deserialization fail to properly unpack these lists in a way that maintains the TrialStatuses type. Here we add a special method to do so. 3. All other transitioncriterion can continue using deserialize method There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time. Reviewed By: sdaulton, saitcakmak Differential Revision: D55727618
mgarrard
force-pushed
the
export-D55727618
branch
from
325c5bd
to
6752144
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55727618 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2320 +/- ##
=======================================
Coverage 94.90% 94.90%
=======================================
Files 489 489
Lines 47752 47756 +4
=======================================
+ Hits 45320 45324 +4
Misses 2432 2432 ☔ View full report in Codecov by Sentry. |
|
This pull request has been merged in 05eb25f. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
In D52982664 I introduced a circular dep by moving some storage related logic directly onto transitioncriterion class. In the past week and half, this has been pretty annoying for folks on the team (sorry everyone!). This diff fixes that dep by:
There is a larger question here about ideally if a class inherits from SerializationMixin and all it's fields also inherit from SerializationMixin that the deserialization should eloquently handle this. I tried to find a solution for that for a bit in this period, but i kept introducing circular deps and it seems to be a larger undertaking than i have scope for at the time.
Differential Revision: D55727618