Skip to content
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

[ML] Prevent adding new seasonal and calendar components to comply with hard limit #2469

Merged
merged 20 commits into from May 27, 2023

Conversation

valeriy42
Copy link
Contributor

@valeriy42 valeriy42 commented Apr 4, 2023

This PR improves anomaly detection job compliance with the hard memory limit. It does this by ensuring that the anomaly detection job does not add new seasonal and calendar components to the probability model while it is in the hard_limit state.

See also #373.

@droberts195 droberts195 added v8.9.0 and removed v8.8.0 labels Apr 26, 2023
@valeriy42 valeriy42 changed the title [ML] Respect hard limit [ML] Prevent adding new seasonal and calendar components to comply with hard limit May 16, 2023
@valeriy42 valeriy42 requested a review from tveasey May 16, 2023 12:14
@valeriy42 valeriy42 marked this pull request as ready for review May 16, 2023 12:14
Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I think there is a gotcha though in using unsigned arithmetic to compute extra memory.

include/maths/common/CModel.h Outdated Show resolved Hide resolved
include/maths/common/CModel.h Outdated Show resolved Hide resolved
include/maths/time_series/CTimeSeriesDecompositionDetail.h Outdated Show resolved Hide resolved
include/model/CModelFactory.h Outdated Show resolved Hide resolved
lib/maths/time_series/CTimeSeriesDecompositionDetail.cc Outdated Show resolved Hide resolved
lib/maths/time_series/CTimeSeriesDecompositionDetail.cc Outdated Show resolved Hide resolved
lib/maths/time_series/CTimeSeriesDecompositionDetail.cc Outdated Show resolved Hide resolved
lib/maths/time_series/CTimeSeriesDecompositionDetail.cc Outdated Show resolved Hide resolved
lib/maths/time_series/CTimeSeriesTestForSeasonality.cc Outdated Show resolved Hide resolved
@valeriy42 valeriy42 requested a review from tveasey May 24, 2023 15:00
@valeriy42
Copy link
Contributor Author

@tveasey Thank you for your comments. I addressed all of them. It would be great if you could take another look.

Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -365,6 +366,8 @@ void CEventRateModel::sample(core_t::TTime startTime,
annotationCallback(annotation);
});

params.memoryCircuitBreaker(CMemoryCircuitBreaker(resourceMonitor));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: couldn't this just be appended to the list above?

@@ -525,6 +526,7 @@ void CEventRatePopulationModel::sample(core_t::TTime startTime,
LOG_TRACE(<< "Model unexpectedly null");
continue;
}
params.memoryCircuitBreaker(CMemoryCircuitBreaker(resourceMonitor));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: as before.

@valeriy42
Copy link
Contributor Author

retest

@valeriy42 valeriy42 merged commit 8ad53aa into elastic:main May 27, 2023
10 of 11 checks passed
@valeriy42 valeriy42 deleted the hard-limit-limit branch May 27, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants