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

The prediction menu installation instructions are not correct and should not be used #2092

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

deeleeramone
Copy link
Contributor

@deeleeramone deeleeramone commented Jul 15, 2022

Tensorflow cannot be installed via Poetry. tensorflow-macOS and tensorflow-metal perform terribly and should not be used. Installing tensorflow via conda install -c conda-forge tensorflow is the only way tensorflow should be installed, and it should be the very last thing to be installed.

This PR updates the Anaconda installation instructions to reflect this reality.

deeleeramone and others added 2 commits July 15, 2022 09:49
…uld not be used.

Tensorflow *cannot* be installed via Poetry. `tensorflow-macOS` and `tensorflow-metal` perform terribly and should not be used. Installing tensorflow via `conda install -c conda-forge tensorflow` is the only way tensorflow should be installed.
@DidierRLopes DidierRLopes added the docs Code documentation label Jul 18, 2022
@DidierRLopes
Copy link
Collaborator

@martinb-bb is this mergeable?

@martinb-ai
Copy link
Contributor

@DidierRLopes Yes, this can be merged for now as tested by @deeleeramone on his systems.

I will be updating this doc in the upcoming month with the release of the forecasting menu.

@DidierRLopes
Copy link
Collaborator

Feel free to accept PR so we can merge then 😄

@martinb-ai
Copy link
Contributor

On second thought, @deeleeramone @DidierRLopes should we also clean up the poetry and requirements to remove anything from prediction-m1-mac.

If we are doing install of tensorflow through conda, we shouldn't leave the tom files with floating libs.

@DidierRLopes
Copy link
Collaborator

On second thought, @deeleeramone @DidierRLopes should we also clean up the poetry and requirements to remove anything from prediction-m1-mac.

If we are doing install of tensorflow through conda, we shouldn't leave the tom files with floating libs.

Sounds good to me. @piiq anything against?

@martinb-ai
Copy link
Contributor

martinb-ai commented Jul 18, 2022

Also, tensorflow installs fine with poetry on Linux and Windows. This seemed to be a mac issue. I think uses still can use poetry as long as they are non-mac

The way it was structured before worked fine.

For MACs only:

poetry install -E prediction
conda install -c conda-forge tensorflow

@martinb-ai
Copy link
Contributor

martinb-ai commented Jul 18, 2022

This is what I had on the readme before:

    #To enable the prediction menu install additional dependencies after installing main dependencies:
    
    #Intel mac

    conda install -c conda-forge tensorflow==2.7.0
    conda install -c conda-forge -c pytorch u8darts-torch
    poetry install -E prediction

    #M1 mac (make sure you are using miniconda3 or miniforge3)

    brew install cmake
    brew install gcc
    conda install -c apple tensorflow-deps
    conda install -c apple tensorflow-deps==2.8.0
    python -m pip install tensorflow-metal
    conda update pip
    conda update -c defaults numpy
    export GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1
    export GRPC_PYTHON_BUILD_SYSTEM_ZLIB=1
    wget https://raw.githubusercontent.com/Homebrew/homebrew-core/fb8323f2b170bd4ae97e1bac9bf3e2983af3fdb0/Formula/libomp.rb
    brew unlink libomp
    brew install libomp.rb
    pip3 install -r requirements.txt
    conda install -c conda-forge tensorflow==2.8.0
    conda install -c conda-forge -c pytorch u8darts-torch
    python3 -c "import tensorflow as tf; print(tf.reduce_sum(tf.random.normal([1000, 1000])))"

    # all other systems (Linux/Windows)

    poetry install -E prediction
    conda install -c conda-forge -c pytorch u8darts-torch

We can just change the M1 part to:

poetry install -E prediction
conda install -c conda-forge tensorflow

@piiq
Copy link
Contributor

piiq commented Jul 18, 2022

@deeleeramone's instructions presented in this PR work for all platforms and enable CPU-only computation. The argument in the PR is valid for the use cases that we have. We don't have batches bit enough to use hardware acceleration (be it with CUDA on Nvidia's chips or MPS on Apple's chips). The CPU-only is winning performance wise with the TF functions that we have and we are looking to move to torch (1.12.0 supports all platforms with and without acceleration and is installable from pip) in the #1933.

The prediction and the m1-prediction extras are obsolete with the command from the docs, but we should not delete them for now as this place of the toml is being worked on to make the terminal and the api "pip-installable"

@martinb-ai
Copy link
Contributor

I can agree with that. Approved

@piiq piiq merged commit e8b2e9c into OpenBB-finance:main Jul 18, 2022
@deeleeramone deeleeramone deleted the patch-15 branch January 7, 2023 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Code documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants