-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Use more reliable method to run homebrew-pypi-poet #9391
Conversation
Review period will end on 2020-12-03 at 20:32:43 UTC. |
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.
Looks good assuming this worked as expected for you (it sounds like it did). I don't actually use pipenv
so I can't really judge that part of it.
Just one question, is pipenv
needed or can this be accomplished using a standard virtual environment (i.e. without the need to install pipenv
).
Here's what seems to work for me:
# Use a temporary directory for the virtual environment
cd "$(mktemp -d)"
# Create and source a new virtual environment in the venv/ directory
python3 -m venv venv
source venv/bin/activate
# Install the package of interest as well as homebrew-pypi-poet
pip install some_package homebrew-pypi-poet
poet some_package
# Destroy the virtual environment
deactivate
rm -rf venv
And similarly (once in the virtual environment):
pip install homebrew-pypi-poet
poet -s requirements.in
Edit: I noticed this and commented in Slack, but figured I'd relay it here:
I don't know how common requirements.in
files are, but the one that I saw did not contain any version info. This leads to poet
always retrieving the latest version of the package which could, potentially, ignore version pinning/rules that exist in requirements.txt
. For example, requirements.in
may contain:
boltons
While requirements.txt
may contain:
boltons==17.2.0
Using method number two, poet
will retrieve the latest boltons
version (20.2.1 as I'm writing this) instead of version 17.2.0. Because of this, we may not want to encourage people to use this method as it may lead to resource updates that are incorrect.
I tried your suggestion and it fails for me:
You’re right. I have removed that section. Thanks for the pointer! |
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'd rather we figure out how to fix the original instructions rather than add an "alternatively" that may diverge over time and we're not really sure will work any more consistently.
My tests on two different devices and OS versions have established that it does work more consistently than the other two alternatives (i.e. the original one and the one @Rylan12 has suggested). I’m not sure we’re not overthinking this. Are we supposed to be risk averse with regards to the core software and core formulae? Yes, absolutely. Shall we be risk averse at updating out-of date documentation? I don’t know. We can always revert later if my variant turns out to be wrong.
I wouldn’t mind just replacing the original instructions. |
Replace the `virtualenvwrapper` method with one based on the `pipenv` formula, a dependency manager but doubles as a `virtualenvwrapper` alternative. A non-brewed installation of virtualenvwrapper is not always reliable. For me, it fails like this: ``` $ source /Users/claudia/Library/Python/3.9/bin/virtualenvwrapper.sh /usr/bin/python: No module named virtualenvwrapper virtualenvwrapper.sh: There was a problem running the initialization hooks. If Python could not import the module virtualenvwrapper.hook_loader, check that virtualenvwrapper has been installed for VIRTUALENVWRAPPER_PYTHON=/usr/bin/python and that PATH is set properly. ```
@MikeMcQuaid Done; I replaced the original instructions. |
I'm risk averse in adding an "alternatively" instead of either fixing whatever is broken originally.
Yup, this seems like a good idea if someone else can verify this. I've removed my objections to this PR being merged but I don't have enough context to comment on the changes here. |
Fair enough. Thanks for your thoughts and for the update @MikeMcQuaid 👍 |
I will take a look at this later today. It's worth noting, though, that my testing is not done from a clean install and so my environment may influence the results. Already, it seems that Claudia and I have had different results when trying an alternative solution. See #9391 (review) and #9391 (comment) |
Review period ended. |
Okay, here are my results. I will test three things:
Before each trial, I will reset my environment by running $ python3 -m pip list
Package Version
---------- -------
pip 20.3
setuptools 50.3.2
wheel 0.35.1 The package that I will be testing with is Note: I've included shortened versions of the outputs of each command. You can view the entire output here. 1. The existing method from the docs$ python3 -m pip install virtualenvwrapper
<installs successfully>
$ source $(brew --prefix)/bin/virtualenvwrapper.sh
<runs successfully>
$ mktmpenv
<runs successfully>
$ pip3 install snakemake homebrew-pypi-poet
<installs successfully>
$ poet snakemake
<outputs packages as expected>
$ deactivate
<removed virtual environment as expected> Verdict: Success 2. @claui's method documented in this PR$ brew reinstall pipenv
<installs successfully>
$ cd "$(mktemp -d)"
<runs successfully>
$ pipenv install snakemake homebrew-pypi-poet
<installs successfully>
$ pipenv run poet snakemake
<outputs packages as expected>
$ pipenv --rm
<runs successfully> Verdict: Success 3. My method documented in #9391 (review)$ cd "$(mktemp -d)"
<runs successfully>
$ python3 -m venv venv
<runs successfully>
$ source venv/bin/activate
<runs successfully>
$ pip3 install snakemake homebrew-pypi-poet
<installs successfully>
$ poet snakemake
<outputs packages as expected>
$ deactivate
<runs successfully> Verdict: Success ConclusionThese all seem to be fine with me. However, the |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@Rylan12 Does the |
@MikeMcQuaid This PR isn’t stale. |
@claui The existing method works for @Rylan12, the documented method requires fixes before it will work and @Rylan12 suggested another method. I don't think it's worth progressing with this as a result. There's been no activity for 25 days beyond it being marked as stale and no activity since then hence my closing. |
@MikeMcQuaid What tag should I have used, other than do not merge, to indicate that this is blocked by an unrelated issue?
I was away from my computer for the holidays.
and fails for me.
which @fxcoudert has been addressing in the (unrelated) Python 3.9 issue.
which fails for me. I’d like to confirm that the method this PR proposes is the only one that works reliably. |
Since the last time I looked at this, the Python issue mentioned has been fixed. I went back and re-attempted @claui's proposed fix and it worked fine with the fixed Python 3.9. I've updated my comment above to reflect this. |
Thanks @Rylan12 for the update and for re-trying. And apologies @MikeMcQuaid for my lack of responsiveness. |
@MikeMcQuaid With the proposed method now confirmed to work for both Rylan and myself, please consider re-opening. Thank you. |
I'd have expected a comment from the PR author noting that.
I'm fine with this being merged as-is. |
Pinging @dtrodrigues for a third opinion (I just saw #10039.) |
From the existing documentation, the first method of It's worth noting that the dependency resolution of |
Is changing the command to Also, is there a reason to use In terms of the dependency resolver, is this going to cause issues? As in, should |
Just Longer-term, the divergence between |
Within a |
@claui can you try the method I suggested in #9391 (review) again, this time adding |
@Rylan12 Yay – that works! 🎉 Also:
@dtrodrigues You’re absolutely right.
Trashing the @dtrodrigues @Rylan12 Is it worth including |
Now that it works for everyone, I absolutely agree. |
@Rylan12 Do you want to open a PR with your solution (and get proper credit) or would you prefer me to do it? |
@claui Happy to open a new PR (although I wouldn't have been upset at all if you went ahead and made the changes in here)! My gut is that it's a pretty specific problem so unless more users report issues, I'm not sure we need to include it. I have no problem including it, though, if we think it's worth it. |
brew style
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?Update: Replacing the old method instead of adding to it.
This replaces the
virtualenvwrapper
method in the documentation with one based on thepipenv
formula, a dependency manager that doubles as avirtualenvwrapper
alternative.A non-brewed installation of virtualenvwrapper is not always reliable.
For me, it fails like this:
It’s therefore useful to document a working alternative based on thepipenv
formula, which is actually a dependency manager but doubles as avirtualenvwrapper
alternative.This also adds documentation on how to usepoet
on software that is not on PyPI.Thanks again to @Rylan12 for technical help!