Skip to content

Conversation

@mpolson64
Copy link
Contributor

Summary:
Misc improvements and tricks to make DataRows more performant. We're within spitting distance of the original implementation with dataframes, close enough that Im willing to consider the difference is likely due to scheduler noise; IMO good enough to land.

  1. Removed isinstance check from Data init -- this was helpful when refactoring since some calls to Data(df) didnt use kwargs and caused errors, but added unnecessary overhead
  2. [BIG IMPROVEMENT] Used df.itertuples instead of df.iterrows in Data init when initializing from a dataframe. This alone took us from 1h 44m to ~40m
  3. New empty, metric_names, and trial_indices properties which dont require constructing full_df
  4. Changes to Experiment.attach_data which operate directly on list[DataRows] instead of on DataFrames (ie migrating from combine_df_favoring_recent helper fn to new combine_data_rows_favoring_recent fn)
  5. Changed [*foo] to list(foo) in a couple places. Metamate tells me this is faster in extremely high data regimes -- not sure I notice a difference or trust it necessarily.

Remaining TODOs:
Id be interested in removing property from the methods which are not O(1); theres a lot of fairly expensive things we do in Data, or at least things which require a full scan, which look like they should be fast because they have the same syntax as an attribute lookup. If nobody has any objections to this Ill ask Metamate to do this for us

Differential Revision: D90713603

@meta-cla meta-cla bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 15, 2026
@meta-codesync
Copy link

meta-codesync bot commented Jan 15, 2026

@mpolson64 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D90713603.

mpolson64 added a commit to mpolson64/Ax that referenced this pull request Jan 15, 2026
Summary:
Pull Request resolved: facebook#4774

Misc improvements and tricks to make DataRows more performant. We're within spitting distance of the original implementation with dataframes, close enough that Im willing to consider the difference is likely due to scheduler noise; IMO good enough to land.

1. Removed isinstance check from Data init -- this was helpful when refactoring since some calls to Data(df) didnt use kwargs and caused errors, but added unnecessary overhead
2. **[BIG IMPROVEMENT]** Used df.itertuples instead of df.iterrows in Data init when initializing from a dataframe. This alone took us from 1h 44m to ~40m
3. New empty, metric_names, and trial_indices properties which dont require constructing full_df
4. Changes to Experiment.attach_data which operate directly on list[DataRows] instead of on DataFrames (ie migrating from combine_df_favoring_recent helper fn to new combine_data_rows_favoring_recent fn)
5. Changed [*foo] to list(foo) in a couple places. Metamate tells me this is faster in extremely high data regimes -- not sure I notice a difference or trust it necessarily.

Remaining TODOs:
Id be interested in removing `property` from the methods which are not O(1); theres a lot of fairly expensive things we do in Data, or at least things which require a full scan, which look like they should be fast because they have the same syntax as an attribute lookup. If nobody has any objections to this Ill ask Metamate to do this for us

Differential Revision: D90713603
Summary:

TData was necesssary whern we had multiple different Data classes, but recent developments have made this no longer needed

Differential Revision: D90596942
Summary:

Moved these tests into TestData, since Data is the only data-related class in Ax.

Differential Revision: D90605845
Summary:

NOTE: This is much slower than the implementation which is backed by a dataframe. For clarity, Ive put this naive implementation up as its own diff and the next diff hunts for speedups.

Creates new source of truth for Data: the DataRow. The df is now a cached property which is dynamically generated based on these rows.

In the future, these will become a Base object in SQLAlchemy st. Data will have a SQLAlchemy relationship to a list of DataRows which live in their own table.

RFC:

1. Im renaming sem -> se here (but keeping sem in the df for now, since this could be an incredibly involved cleanup). Do we have alignment that this is a positive change? If so I can either start of backlog the cleanup across the codebase. cc Balandat who Ive talked about this with a while back.
2. This removes the ability for Data to contain arbitrary columns, which was added in D83682740 and afaik unused. Arbitrary new columns would not be compatible with the new storage setup (it was easy in the old setup which is why we added it), and I think we should take a careful look at how to store contextual data in the future in a structured way.

Differential Revision: D90605846
Summary:

Misc improvements and tricks to make DataRows more performant. We're within spitting distance of the original implementation with dataframes, close enough that Im willing to consider the difference is likely due to scheduler noise; IMO good enough to land.

1. Removed isinstance check from Data init -- this was helpful when refactoring since some calls to Data(df) didnt use kwargs and caused errors, but added unnecessary overhead
2. **[BIG IMPROVEMENT]** Used df.itertuples instead of df.iterrows in Data init when initializing from a dataframe. This alone took us from 1h 44m to ~40m
3. New empty, metric_names, and trial_indices properties which dont require constructing full_df
4. Changes to Experiment.attach_data which operate directly on list[DataRows] instead of on DataFrames (ie migrating from combine_df_favoring_recent helper fn to new combine_data_rows_favoring_recent fn)
5. Changed [*foo] to list(foo) in a couple places. Metamate tells me this is faster in extremely high data regimes -- not sure I notice a difference or trust it necessarily.

Remaining TODOs:
Id be interested in removing `property` from the methods which are not O(1); theres a lot of fairly expensive things we do in Data, or at least things which require a full scan, which look like they should be fast because they have the same syntax as an attribute lookup. If nobody has any objections to this Ill ask Metamate to do this for us

Differential Revision: D90713603
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant