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

Feature/improve docs #4260

Merged
merged 12 commits into from
Feb 16, 2023
Merged

Feature/improve docs #4260

merged 12 commits into from
Feb 16, 2023

Conversation

DidierRLopes
Copy link
Collaborator

Improve main README and also PyPI page

@reviewpad reviewpad bot added the feat S Small T-Shirt size Feature label Feb 15, 2023
Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

The dependency tree has been changed. Please update requirements.txt and requirements-full.txt

@DidierRLopes DidierRLopes added the docs Code documentation label Feb 15, 2023
Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

The dependency tree has been changed. Please update requirements.txt and requirements-full.txt

Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

The dependency tree has been changed. Please update requirements.txt and requirements-full.txt

Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

The dependency tree has been changed. Please update requirements.txt and requirements-full.txt

@DidierRLopes
Copy link
Collaborator Author

@jmaslek is there anything else you want me to add here?

reviewpad[bot]
reviewpad bot previously requested changes Feb 16, 2023
Copy link

@reviewpad reviewpad bot left a comment

Choose a reason for hiding this comment

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

The dependency tree has been changed. Please update requirements.txt and requirements-full.txt

@JerBouma
Copy link
Contributor

I just want to say again that the installation instructions could result in issue as they are put there. We ask people to create an environment in here (https://docs.openbb.co/sdk/quickstart/installation) for a reason. If I'd think "this is just another package" and I do pip install openbb in my personalised environment that results into many conflicts most likely.

@DidierRLopes
Copy link
Collaborator Author

DidierRLopes commented Feb 16, 2023

I just want to say again that the installation instructions could result in issue as they are put there. We ask people to create an environment in here (https://docs.openbb.co/sdk/quickstart/installation) for a reason. If I'd think "this is just another package" and I do pip install openbb in my personalised environment that results into many conflicts most likely.

Is this still required even with portfolio optimization and forecasting out of scope?

@JerBouma
Copy link
Contributor

You are still blasting the environment with https://github.com/OpenBB-finance/OpenBBTerminal/blob/develop/requirements.txt with specific Pandas, NumPy etc. For some packages we stay on older version on purpose. If pip doesn't entirely recognize that stuff, it could be that they can't launch the Terminal or SDK because of dependency errors. I believe this was the main reason we ask people to make an environment but please @jmaslek or @piiq confirm this.

@piiq
Copy link
Contributor

piiq commented Feb 16, 2023

pip looks at the dependency range specified in pyproject.toml and installs the dependencies accordingly.
When you manually pip install openbb into an existing environment that has same packages that openbb depends on, if the version of an existing package does not match the range specified in pyproject.toml - the existing package will be overwritten.
This will happen with any package that you install with pip not openbb specifically. does that answer your question @JerBouma

Let's outline what should we improve in the docs

@JerBouma
Copy link
Contributor

So to me this is a problem if you would install the package within an environment that is already filled with different packages. Is it not? Otherwise, why do we mention in the Docs to create an environment?

@piiq
Copy link
Contributor

piiq commented Feb 16, 2023

Making a reproducible environment with a dependency tree that works nicely is a problem for everyone, not just you. This problem triggered an inception of a universe of tools. Examples of such tools include general purpose containerization tools like docker and podman and tools from the python ecosystem like conda, pyenv and poetry

My experience for creating a vanilla pip environment is to have openbb as the primary dependency and only installing on top whatever is not available in openbb. I haven't yet encountered a situation when I would specifically need a different version of some package than the one defined by openbb

@JerBouma
Copy link
Contributor

JerBouma commented Feb 16, 2023

Alright, thank you for the explanation. My question then is to you and @DidierRLopes:

Does it make sense that on the PyPi page we say just do pip install openbb and in the Docs we say "make environment etc etc". If not, let's at least say the same thing on both then and in that case, which one?

And in that case, it needs to be part of the release schedule to update this doc as well when we do make changes.

@piiq
Copy link
Contributor

piiq commented Feb 16, 2023

Tecnically pypi is a place to host a python package. The python package is installed with a pip command. So technically what is written here makes and it's out of scope of package installation to advice a specific system setup.

I would point people to the docs from the pypi page. Disregarding very minor style/lingo errors our docs are pretty explicit/transparent about why people should use virtual environments and a recommended way to set one up.

Copy link
Contributor

@piiq piiq left a comment

Choose a reason for hiding this comment

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

Re-read pypi.md
I think this is just fine @JerBouma

@JerBouma
Copy link
Contributor

Ok all good then :)

@piiq piiq dismissed reviewpad[bot]’s stale review February 16, 2023 11:01

Changes to pyproject.toml don't touch the dependencies

@piiq piiq merged commit 2130dea into develop Feb 16, 2023
@piiq piiq deleted the feature/improve-docs branch February 16, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Code documentation feat S Small T-Shirt size Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants