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

Dev/upgrade prophet 1.1 #627

Merged
merged 4 commits into from Sep 27, 2022
Merged

Conversation

jklaise
Copy link
Member

@jklaise jklaise commented Sep 21, 2022

Resolves #626.

Went straight to 1.1 as they have nice things:

  • No dependency on pystan due to stopped development, the stan backend used now is cmdstanpy
  • Wheels provided for all major platforms and Python versions so no installation woes, means we should be able to run Windows tests

I've tested also that the sole example notebooks works as expected.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Merging #627 (341af20) into master (6274a96) will not change coverage.
The diff coverage is n/a.

❗ Current head 341af20 differs from pull request most recent head c1531dc. Consider uploading reports for the commit c1531dc to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #627   +/-   ##
=======================================
  Coverage   77.96%   77.96%           
=======================================
  Files         123      123           
  Lines        8749     8749           
=======================================
  Hits         6821     6821           
  Misses       1928     1928           
Flag Coverage Δ
macos-latest-3.10.6 74.54% <ø> (ø)
ubuntu-latest-3.10.6 77.34% <ø> (ø)
ubuntu-latest-3.7 77.75% <ø> (ø)
ubuntu-latest-3.8 77.79% <ø> (ø)
ubuntu-latest-3.9 77.79% <ø> (ø)
windows-latest-3.9 74.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ascillitoe
Copy link
Contributor

  • No dependency on pystan due to stopped development, the stan backend used now is cmdstanpy
  • Wheels provided for all major platforms and Python versions so no installation woes, means we should be able to run Windows tests

These would both be great!

fi
if [ "$RUNNER_OS" == "Linux" ]; then # Currently, we only support KeOps on Linux.
python -m pip install --upgrade --upgrade-strategy eager -e .[prophet]
if [ "$RUNNER_OS" == "Linux"]; then # Currently, we only support KeOps on Linux.
Copy link
Contributor

Choose a reason for hiding this comment

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

The test coverage has dropped because the keops tests are not running. I suspect this is because a space is missing in between "Linux" and ] here:

if [ "$RUNNER_OS" == "Linux"]

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot, will change it back. Weitd think is I don't think I edited that line, my editor must've done it automatically which is not great...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeh I only know about this issue as ran into the same thing previously. Now I'm wondering if the editor did it last time too...

setup.py Show resolved Hide resolved
@jklaise
Copy link
Member Author

jklaise commented Sep 22, 2022

I've also updated the changelog (note removal of a couple of extraneous lines from previous releases).

Also note that I've manually checked the save/load functionality still works (we don't have a test for it, happy to add?).

@ascillitoe
Copy link
Contributor

Also note that I've manually checked the save/load functionality still works (we don't have a test for it, happy to add?).

When you say save/load functionality, what are you referring to? Saving and loading OutlierProphet via save_detector and load_detector?

Copy link
Contributor

@ascillitoe ascillitoe left a comment

Choose a reason for hiding this comment

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

LGTM!

@jklaise
Copy link
Member Author

jklaise commented Sep 22, 2022

Also note that I've manually checked the save/load functionality still works (we don't have a test for it, happy to add?).

When you say save/load functionality, what are you referring to? Saving and loading OutlierProphet via save_detector and load_detector?

Yup, that's the one.

@ascillitoe
Copy link
Contributor

When you say save/load functionality, what are you referring to? Saving and loading OutlierProphet via save_detector and load_detector?

Yup, that's the one.

There is actually one test for it in the legacy save/load tests (since outlier detectors are still saved in the legacy format):

if not isinstance(OutlierProphet, MissingDependency):
detector.append(
OutlierProphet(threshold=.7,
growth='logistic')
)

We'll need to write a new one in saving/tests/test_saving.py once we've updated serialization for outlier detectors.

Copy link
Contributor

@arnaudvl arnaudvl left a comment

Choose a reason for hiding this comment

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

LGTM!

@ascillitoe ascillitoe merged commit df7edca into SeldonIO:master Sep 27, 2022
ascillitoe pushed a commit that referenced this pull request Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update fbprophet versions
4 participants