-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore(docs): reorder, clarify PyPI installation #28221
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I am not an official review/maintainer. As you are requested for feedback added my comments
Those are great additions, thanks @surapuramakhil ! |
I agree, Configuring Superset should be about options available after Installation, much like day-to-day administration of a environment. |
Had accidentally removed it in a prior commit instead of moving it
@artofcomputing I have added this to the BugHerd task board. |
docs/docs/installation/pypi.mdx
Outdated
# See https://docs.python.org/3.6/library/venv.html | ||
python3 -m venv venv | ||
. venv/bin/activate | ||
mkdir superset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. I'm a tad confused. Shouldn't the virtualenv be called venv
—assuming it's inside the root superset
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the docs don't say to make a folder called superset
, that's something I'm adding here. I believe what they say currently makes a virtualenv called venv
in the root directory, which is not descriptive and potentially a problem if another tutorial says to do the same.
Now maybe mkdir superset
is unnecessary if python3 -m venv superset
below would create the directory.
I may be misunderstanding though, I'm new to this aspect.
docs/docs/installation/pypi.mdx
Outdated
``` | ||
|
||
Or with pyenv-virtualenv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want to keep pyenv
documentation around, i.e., developers may want to switch which Python environment they want to use. In essence pyenv
is a precursor to setting up Superset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I'm new to this but my newbie perspective was: the docs say:
You can create and activate a virtual environment using: [virtual env instructions] ... Or with pyenv-virtualenv:
I was unclear, which should I use? And when I read a little about it, it seemed like the venv
was the most widely-used and so I eliminated an alternative.
If we should include both, can someone add a note to help a user decide which to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley circling back to this -- are you good with my changes or is there another committer you think could weigh in? Or if nothing else I can revert these parts, I'd rather just get the bulk of this merged.
Do we still need eyes on this @sfirke? Sorry it seems to have slipped under the collective radar. |
@rusackas yes this still needs a review. It's tricky b/c I'm not enough of an expert to feel 100% I'm right but also I'm confident this improves on docs steps that are straight-up missing or out of order. |
I'm a totally new Superset user (on linux) and ran into problems when following the PyPI installation instructions that are in the live docs. This PR is super useful, as it shows what the missing configuration steps are. The instructions clarified things a lot and worked well, but in the end I called the venv folder "venv" instead, as that's what I'm used to. I'm also used to these virtual environment folders being totally rebuildable by
Specifically:
Setting up with example data and running development server:
(The SUPERSET_CONFIG_PATH environment variable still needs to be set for the above to work.) It might be helpful to add a comment about it not in general being a good idea to set |
@olof-dev that is very helpful feedback, thank you! I will incorporate that, including naming the venv |
A commenter in Slack shares this feedback:
|
Taking stock today: I need to incorporate feedback from @olof-dev above and the Slack comment, then let's merge this as an improvement on what we have now, even if it's imperfect. |
Committing this to resolve the comment and re-kick CI.
SUMMARY
I tried installing from PyPI and found that some steps were missing or out of order. I removed irrelevant text, added other instructions, and reordered things.
Any concerns about suggesting that people install into a directory
/superset
and set up their virtual environment there?There's now some redundancy with Configuring Superset docs, as they also talk about creating a
superset_config.py
file. Because the proper time to create one is in the install phase, when you are specifying aSECRET_KEY
, I feel this belongs in this page. I think we should add the corresponding creating-a-config content to the install methods, then remove from Configuring Superset as everyone will have done it by that point.TESTING INSTRUCTIONS
Follow these steps on a fresh Linux machine and see if they work. Please test!