Skip to content

[ML] Prepare parsing phase_progress from DFA process #55580

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

Conversation

dimitris-athanasiou
Copy link
Contributor

Data frame analytics process currently reports progress as
an integer progress_percent. We parse that and report it
from the _stats API as the progress of the analyzing phase.
However, we want to allow the DFA process to report progress
for more than one phase. This commit prepares for this by
parsing phase_progress from the process, an object that
contains the phase name plus the progress_percent for that
phase.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@przemekwitek przemekwitek self-requested a review April 22, 2020 10:34
Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

Looks good, just two minor remarks.

@@ -58,6 +62,8 @@
}

private final RowResults rowResults;
// TODO remove after process is writing out phase_progress
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this TODO be for line 67?

@@ -164,11 +165,18 @@ private void processResult(AnalyticsResult result, DataFrameRowsJoiner resultsJo
if (rowResults != null) {
resultsJoiner.processRowResults(rowResults);
}
PhaseProgress phaseProgress = result.getPhaseProgress();
if (phaseProgress != null) {
LOGGER.debug("[{}] Analyzing progress updated to [{}]", analytics.getId(), phaseProgress.getProgressPercent());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also print the phase?

Data frame analytics process currently reports progress as
an integer `progress_percent`. We parse that and report it
from the _stats API as the progress of the `analyzing` phase.
However, we want to allow the DFA process to report progress
for more than one phase. This commit prepares for this by
parsing `phase_progress` from the process, an object that
contains the `phase` name plus the `progress_percent` for that
phase.
@dimitris-athanasiou dimitris-athanasiou force-pushed the parse-phase-progress-from-analytics-process branch from 8138f15 to a69f355 Compare April 22, 2020 12:02
@dimitris-athanasiou dimitris-athanasiou merged commit 2d55592 into elastic:master Apr 22, 2020
@dimitris-athanasiou dimitris-athanasiou deleted the parse-phase-progress-from-analytics-process branch April 22, 2020 12:46
dimitris-athanasiou added a commit that referenced this pull request Apr 22, 2020
…55587)

Data frame analytics process currently reports progress as
an integer `progress_percent`. We parse that and report it
from the _stats API as the progress of the `analyzing` phase.
However, we want to allow the DFA process to report progress
for more than one phase. This commit prepares for this by
parsing `phase_progress` from the process, an object that
contains the `phase` name plus the `progress_percent` for that
phase.

Backport of #55580
dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Apr 24, 2020
Since elastic#55580 we've introduced a new format for parsing progress
from the data frame analytics process. As the process is now
writing out progress in this new way, we can remove the parsing
of the old format.
dimitris-athanasiou added a commit that referenced this pull request Apr 24, 2020
Since #55580 we've introduced a new format for parsing progress
from the data frame analytics process. As the process is now
writing out progress in this new way, we can remove the parsing
of the old format.
dimitris-athanasiou added a commit that referenced this pull request Apr 24, 2020
…) (#55720)

Since #55580 we've introduced a new format for parsing progress
from the data frame analytics process. As the process is now
writing out progress in this new way, we can remove the parsing
of the old format.

Backport of #55711
dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Apr 25, 2020
This is a continuation from elastic#55580.

Now that we're parsing phase progresses from the analytics process
we change `ProgressTracker` to allow for custom phases between
the `loading_data` and `writing_results` phases. Each `DataFrameAnalysis`
may declare its own phases.

This commit sets things in place for the analytics process to start
reporting different phases per analysis type. However, this is
still preserving existing behaviour as all analyses currently
declare a single `analyzing` phase.
dimitris-athanasiou added a commit that referenced this pull request Apr 27, 2020
This is a continuation from #55580.

Now that we're parsing phase progresses from the analytics process
we change `ProgressTracker` to allow for custom phases between
the `loading_data` and `writing_results` phases. Each `DataFrameAnalysis`
may declare its own phases.

This commit sets things in place for the analytics process to start
reporting different phases per analysis type. However, this is
still preserving existing behaviour as all analyses currently
declare a single `analyzing` phase.
dimitris-athanasiou added a commit that referenced this pull request Apr 27, 2020
) (#55791)

This is a continuation from #55580.

Now that we're parsing phase progresses from the analytics process
we change `ProgressTracker` to allow for custom phases between
the `loading_data` and `writing_results` phases. Each `DataFrameAnalysis`
may declare its own phases.

This commit sets things in place for the analytics process to start
reporting different phases per analysis type. However, this is
still preserving existing behaviour as all analyses currently
declare a single `analyzing` phase.

Backport of #55763
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.

4 participants