simplify Dockerfile #113
Merged
simplify Dockerfile #113
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
## Overview This PR uses updated versions of Python and `prophet` to greatly simplify the python environment setup in the Dockerfile. The code has been tested by creating a local Docker container, and sample outputs were written to the following tables in `moz-fx-data-bq-data-science.bochocki`: - `tmp_desktop_kpi_forecast` - `tmp_desktop_kpi_forecast_confidences` - `tmp_mobile_kpi_forecast` - `tmp_mobile_kpi_forecast_confidences` ## Additional Changes - `.gitignore`: ignore additional filetypes - `kpi_forecasting.py`: set confidence intervals `target` from `config` instead of relying on hardcoded `"desktop"`. This `target` is overwritten in `write_confidence_intervals_to_bigquery` [here](https://github.com/mozilla/docker-etl/blob/4cfbec915375343023944d1ca23f527251a5ada8/jobs/kpi-forecasting/kpi-forecasting/Utils/DBWriter.py#L116), but I think this change makes the it clear that we're not unintentionally using "desktop" labels on "mobile" forecasts. - `PosteriorSampling.py`: minor refactoring required to resolve errors and deprecation warnings that are now being raised by pandas as a result of package upgrades. - `README.md`: update examples - `requirements.txt`: updated packages to get easier-install versions of `prophet` and `statsforecast`.
bochocki
commented
May 9, 2023
| @@ -31,10 +31,9 @@ def get_confidence_intervals( | |||
| uncertainty_samples["ds"] > np.datetime64(final_observed_sample_date) | |||
| ] | |||
| .groupby("{}".format(aggregation_unit_of_time)) | |||
| .sum() | |||
| .sum(numeric_only=True) | |||
There was a problem hiding this comment.
bochocki
commented
May 9, 2023
Comment on lines
+89
to
+91
| uncertainty_samples_aggregated.iloc[0, 1:] += observed_aggregated["value"].iloc[ | ||
| -1 | ||
| ] |
There was a problem hiding this comment.
This is the same intended logic as before, but the previous code doesn't work in new versions of pandas because observed_aggregated.iloc[-1].value doesn't return a single value, it returns an array of values. Using the . column access method was also confusing, because at first glance it looks like a typo of .values which casts a pandas column to a numpy array.
bochocki
commented
May 9, 2023
| @@ -71,6 +70,8 @@ def get_confidence_intervals( | |||
| columns={"y": "value"} | |||
| ).sort_values(by="{}".format(aggregation_unit_of_time)) | |||
|
|
|||
| observed_aggregated = observed_aggregated.astype({"value": np.float64}) | |||
There was a problem hiding this comment.
observed_aggregated["value"] is being stored as an Int64Dtype, which is a pandas type for storing large integers. For some reason, using this type breaks the following merge on line 100:
all_aggregated = pd.merge(
observed_aggregated,
uncertainty_samples_aggregated,
on=["{}".format(aggregation_unit_of_time), "value", "type"],
how="outer",
)I think using float64 instead is an okay workaround here, since the values in the confidence intervals are reported as float64 anyways.
perrymcmanis144
approved these changes
May 10, 2023
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.
Overview
This PR uses updated versions of Python and
prophetto greatly simplify the python environment setup in the Dockerfile. The code has been tested by creating a local Docker container, and sample outputs were written to the following tables inmoz-fx-data-bq-data-science.bochocki:tmp_desktop_kpi_forecasttmp_desktop_kpi_forecast_confidencestmp_mobile_kpi_forecasttmp_mobile_kpi_forecast_confidencesAdditional Changes
.gitignore: ignore additional filetypeskpi_forecasting.py: set confidence intervalstargetfromconfiginstead of relying on hardcoded"desktop". Thistargetis overwritten inwrite_confidence_intervals_to_bigqueryhere, but I think this change makes the it clear that we're not unintentionally using "desktop" labels on "mobile" forecasts.PosteriorSampling.py: minor refactoring required to resolve errors and deprecation warnings that are now being raised by pandas as a result of package upgrades.README.md: update examplesrequirements.txt: updated packages to get easier-install versions ofprophetandstatsforecast.Checklist for reviewer:
referenced, the pull request should include the bug number in the title)
.circleci/config.yml) will cause environment variables (particularlycredentials) to be exposed in test logs
telemetry-airflow
responsibly.