Replace _data_by_trial with Data on Experiment
#4529
+180
−244
Conversation
This file contains hidden or 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
meta-cla
bot
added
the
CLA Signed
|
@esantorella has exported this pull request. If you are a Meta employee, you can view the originating Diff in D86452716. |
esantorella
force-pushed
the
export-D86452716
branch
from
7b565f4 to
57e2549
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Dec 18, 2025
Summary: This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` ** Attaching data now is simpler * Added property `data.trial_indices` since there were many checks for `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. ** Note that `add_experiment_id` gets some more substantive changes since there is now one `Data` rather than 1+ `Data` for each trial. **Migration off _data_by_trial**: ~~* To keep tests passing and relevant, I added a property `_data_by_trial`.~~ ~~* But a subsequent diff should take this out and update the tests to reference `Data` instead~~ * I initially added `_data_by_trial` as a property. This is quite dangerous since while `experiment._data_by_trial` cannot be set, `experiment._data_by_trial[0][0] = ...` is allowed and could mislead people into thinking they are updating the data on the trial. `_data_by_trial` ought to be removed. Differential Revision: D86452716
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Dec 26, 2025
Summary: Pull Request resolved: facebook#4529 This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` ** Attaching data now is simpler * Added property `data.trial_indices` since there were many checks for `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. ** Note that `add_experiment_id` gets some more substantive changes since there is now one `Data` rather than 1+ `Data` for each trial. **Migration off _data_by_trial**: ~~* To keep tests passing and relevant, I added a property `_data_by_trial`.~~ ~~* But a subsequent diff should take this out and update the tests to reference `Data` instead~~ * I initially added `_data_by_trial` as a property. This is quite dangerous since while `experiment._data_by_trial` cannot be set, `experiment._data_by_trial[0][0] = ...` is allowed and could mislead people into thinking they are updating the data on the trial. `_data_by_trial` ought to be removed. Differential Revision: D86452716
esantorella
force-pushed
the
export-D86452716
branch
from
57e2549 to
ef9a77d
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Dec 26, 2025
Summary: Pull Request resolved: facebook#4529 This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` ** Attaching data now is simpler * Added property `data.trial_indices` since there were many checks for `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. ** Note that `add_experiment_id` gets some more substantive changes since there is now one `Data` rather than 1+ `Data` for each trial. TODO: * Ensure that serialization and deserialization of empty data works -- this has been a bugbear since empty data is now assumed to be Data. Particularly when there is an existing object in the db Differential Revision: D86452716
esantorella
force-pushed
the
export-D86452716
branch
from
ef9a77d to
d925f6b
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: Pull Request resolved: facebook#4529 This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Differential Revision: D86452716 Reviewed By: saitcakmak
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: Pull Request resolved: facebook#4529 This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Reviewed By: saitcakmak Differential Revision: D86452716
esantorella
force-pushed
the
export-D86452716
branch
from
d925f6b to
18a8f59
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4529 +/- ##
=======================================
Coverage 96.72% 96.72%
=======================================
Files 582 582
Lines 60710 60679 -31
=======================================
- Hits 58721 58692 -29
+ Misses 1989 1987 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
esantorella
force-pushed
the
export-D86452716
branch
from
18a8f59 to
3820f35
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Reviewed By: saitcakmak Differential Revision: D86452716
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Reviewed By: saitcakmak Differential Revision: D86452716
Summary: **Context**: This diff exists to split up a more complex change. The next diff, D86452716, will remove `Experiment`'s attribute `_data_by_trial` and give it an attribute `Data`. That is quite a complex change; in addition to the changes on `Experiment`, it requires storage changes and updating many callsites. As a first step, this diff gives `Experiment` a property `data` (which will become an attribute in D86452716) and updates call sites. It should not be landed alone. **Changes**: * Gives `Experiment` a property `Data` that is equivalent to `.lookup_data()` * Updates many call sites to reference `data` rather than `_data_by_trial` Reviewed By: saitcakmak Differential Revision: D87004539
Summary: This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Reviewed By: saitcakmak Differential Revision: D86452716
esantorella
force-pushed
the
export-D86452716
branch
from
3820f35 to
6b8d5a6
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: This diff gets rid of `Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]` and replaces it with `Experiment.data: Data` **Changes**: * `Experiment` has a `data` attribute and does not have a `_data_by_trial` attribute. I did not hide this attribute since I think it is fine to update `Experiment.data` * Attaching data now is simpler * Reference `data.trial_indices` instead of `_data_by_trial.keys()` * Serialization changes: Basically, everything eventually still becomes _data_by_trial the last step in serialization or the first step in deserialization. The db is not affected. Reviewed By: saitcakmak Differential Revision: D86452716
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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:
This diff gets rid of
Experiment._data_by_trial: dict[int, OrderedDict[int, Data]]and replaces it withExperiment.data: DataChanges:
Experimenthas adataattribute and does not have a_data_by_trialattribute. I did not hide this attribute since I think it is fine to updateExperiment.data** Attaching data now is simpler
data.trial_indicessince there were many checks for_data_by_trial.keys()** Note that
add_experiment_idgets some more substantive changes since there is now oneDatarather than 1+Datafor each trial.Migration off _data_by_trial:
* To keep tests passing and relevant, I added a property_data_by_trial.* But a subsequent diff should take this out and update the tests to referenceDatainstead_data_by_trialas a property. This is quite dangerous since whileexperiment._data_by_trialcannot be set,experiment._data_by_trial[0][0] = ...is allowed and could mislead people into thinking they are updating the data on the trial._data_by_trialought to be removed.Differential Revision: D86452716