Remove Column class for Parameter
#156
Open
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## profile-optimization-db #156 +/- ##
=======================================================
Coverage 87.0% 87.0%
=======================================================
Files 230 230
Lines 8156 8176 +20
=======================================================
+ Hits 7098 7118 +20
Misses 1058 1058
|
This was referenced Jan 31, 2025
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.
Supersedes #151; I'm copying its description etc below.
This PR removes the
Columnclass forParameteras it's not necessary to keep around and even slightly faster to do so. See the following benchmarks:Benchmarks with `Column`
Benchmarks now
It remains to be seen if the rest of the normalization (see #143) is beneficial in terms of performance or not. That may not be urgent, though, as the performance seems to already be better than ixmp's.
TODO:
Of course, one question still remains open: what about the rest of the "normalization" PRs? What about the
.dataattribute? Might it not be faster to also normalize that with its own table?For future reference: I tried (though the branch is not on GitHub). I followed the same approach as in #143 and added a
float"values" column as well as a relationship toUnitas "units" to theParameterDatatable. This worked fine except for one part: theUPSERTfunctionality.With our current implementation, we load the JSON dict into a
pd.DataFrameand then usecombine_first()to achieve an operation that's very similar (if not identical) toUPSERT. With a dedicatedParameterDatatable, we would not need to load the data into memory, but could instead make use of sqlite's and postgres'INSERT ... ON CONFLICT DO UPDATEstructure, which is fully supported by sqlalchemy. This kind of statement requires us to specify theUniqueConstraintor columns that should be checked for a conflict. And this is where the problems come in:TableData, I defined 15 columns for data linked toIndexSets. All of these plus theparameter__idwere grouped in theUniqueConstraint. However, when I ran some test that only added data to two of these columns and specified these columns for the conflict check, sqlalchemy told me it couldn't find aUniqueConstraintfitting these two columns.UniqueConstraint, but that would most likely not work either because most of the columns areNULL. AndNULLs are distinct, so even rows where the non-NULLvalues are identical would be seen as distinct. Postgres offers aNULLS NOT DISTINCTclause, but sqlite doesn't support it and it's generally accepted SQL standard that they are always distinct.NULL, such as"___". However, since this is not intended behaviour, this would not make our DB model more stable.Parameterwith X columns would get a partial unique index for all its rows that is only defined for these X columns. This would likely lead to thousands of indexes on this DB table, which would all be checked for each query to figure out if they are applicable, so would slow down this table over time considerably.NULLcolumns, and make one partition for each category. Each of these might then be able to get a (partial) unique index covering the whole partition, and we would only need to ensure that eachParameterDatacoming in is stored in the right partition. I have not tried this nor checked if partitions generally support having distinct indexes, etc, so I don't know how well this might work.UniqueConstainton each column. Then, specifying two columns would trigger both constraints, but we are not sure how these would be evaluated. Would there be a conflict only in the case where we want it to be, i.e. when both constraints fail, or would the conflict arise already when just one constraint fails, which we would consider premature?Last but not least, even if we figure this problem out, we would be left with a collection that already firmly falls into the category "large" in sqlalchemy as we would frequently want to have several thousand
ParameterDatarows in.data. They offer a few techniques of handling these large collections, but the most prominent seems to be aWriteOnlyCollection, which is not what we need: we often want to read the whole data stored in aParameter.Sqlalchemy also offers various relationship loading techniques. Generally,
lazyis the default andselectinis the most efficient. These can be set per query, which I admittedly did not try, but I set the whole relationship (in the table definition) ofIndexSettoIndexSetDatatoselectin(since that seems to take most of the time in our profiles), and our benchmarks were actually a little slower than withlazy.Note
One thing that might be worth a try still is to revert
indexset.datato a json list and see if that enhances our performance.For the time being, I will not continue work on this. The profiles generated for #150 show that
combine_first()makes up at most 10% of the runtime, almost negligible compared tovalidate_data(). And since we seem to already surpass ixmp in performance, this work can wait at least, if continued at all (due to the problems outlined above).