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

Skip predictions in reports if prediction package is missing. #1900

Merged
merged 11 commits into from
Jun 6, 2022

Conversation

stkerr
Copy link
Contributor

@stkerr stkerr commented Jun 4, 2022

Description

This PR does two main changes:

  • It adds logic to detect if the prediction package is installed and skips all predictions steps if it is not.
  • It explicitly opens the report file as UTF-8, which helps with OS like Windows.

This enables the report to still be generated and used, even if no prediction capabilities are present.

How has this been tested?

I set up a local build with no prediction packages installed and with the feature flag disabled. I then ran the report with reports/equity TSLA and several other tickers. I confirmed that the reports generated successfully and that the prediction tab had a message about the prediction capabilities being missing.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@stkerr stkerr changed the title Add error handling if prediction package is missing. Skip predictions in reports if prediction package is missing. Jun 4, 2022
@stkerr stkerr marked this pull request as ready for review June 4, 2022 19:41
@stkerr
Copy link
Contributor Author

stkerr commented Jun 4, 2022

@jmaslek following up to the bug I opened yesterday - this PR adds logic to check if the prediction module is present and disables the steps that need it if it is not present.

Can you review this PR? Thanks!

@jmaslek jmaslek added bug Fix bug enhancement Enhancement labels Jun 4, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Jun 4, 2022

@jmaslek following up to the bug I opened yesterday - this PR adds logic to check if the prediction module is present and disables the steps that need it if it is not present.

Can you review this PR? Thanks!

Thanks @stkerr! Great catch here. I tested it and the console.print doesn't print to the terminal (which presumably makes sense since its through paper mill). But the report runs without TF installed, so this bug is squashed.

@martinb-bb and @piiq : we should improve the pred api so that certain models are still available in certain cases. For example, the mc techniques are just numpy arrays so it should be usable even if tf is not installed.

@martinb-ai
Copy link
Contributor

That's a great idea @jmaslek! Will look into this.

@piiq piiq merged commit 335b89c into OpenBB-finance:main Jun 6, 2022
@stkerr stkerr deleted the stkerr-prediction-error-handling branch June 6, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants