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

Raise exception when Maven artifacts fail download in setup.py #80

Merged
merged 2 commits into from
Jun 18, 2018

Conversation

jcarey03
Copy link
Contributor

@jcarey03 jcarey03 commented May 2, 2018

Problem
During pip install if a Maven artifact fails to download, the failure is silent (i.e., only a print message occurs). This results in subtle undesirable behavior in KCL workers where a subset of jars exists on the classpath.

Solution
Replace the print message with a RuntimeError for better awareness of failed downloads.

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 2, 2018

Tagging @pfifer for visibility.

@pfifer
Copy link
Contributor

pfifer commented May 3, 2018

It appears the source branch isn't up to date with master. Could you pull master into your branch re-push the PR?

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 4, 2018

@pfifer, sorry about that. We have a fork and it was out of date. Master is now merged. Thanks!

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 7, 2018

@pfifer, would you mind taking another look? Thanks!

@pfifer
Copy link
Contributor

pfifer commented May 7, 2018

Does this cause pip to stop the installation when the download fails?

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 8, 2018

So, a pip install of the dependency directly does in fact fail, which is the desired behavior. This is what I was testing originally, but I noticed upon closer inspection, a pip install with a requirements file does not fail. I'm looking into the difference between the following:

// pip install direct dependency fails correctly
pip -v install git+https://github.com/lyft/amazon-kinesis-client-python@exc_on_failed_jar_download_testing#egg=amazon-kinesis-client-python

// pip requirements file does not fail
pip -v install -r ./requirements.txt

This is probably something obvious, but I'm new Python :)

@pfifer
Copy link
Contributor

pfifer commented May 8, 2018

Thanks just need a license confirmation before I can start merging the change.

Please confirm that we can use, modify, copy, and redistribute this contribution.

Thanks.

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 9, 2018

Thanks. I'll confirm.

Before merging, I want to understand why this doesn't fail when specifying a requirements file. I'm looking into it this week.

@pfifer
Copy link
Contributor

pfifer commented May 9, 2018

I did some testing with requirements.txt and virtualenv. It did throw an error during that testing, it's possible that you didn't actually run your version of the install during the requirements.txt test. My normal testing for installation is something like:

  1. Install the current version, and make sure all the dependencies are installed:
    pip install amazon_kclpy
  2. Remove the current version, this leaves all the dependencies installed:
    pip uninstall amazon_kclpy
  3. Make sure the new version is built:
    python setup.py sdist
  4. Attempt to install the new version:
    pip install amazon_kclpy --no-index --find-links <path to the repo>/amazon-kinesis-client-python/dist

Step 4 can work for requirements.txt as well
pip install -r requirements.txt --no-index --find-links <path to the repo>/amazon-kinesis-client-python/dist

I've had issues with the wheel cache messing this up on Python 3, and if you don't see the process running try adding --no-cache-dir after pip

@jcarey03
Copy link
Contributor Author

jcarey03 commented May 9, 2018

Thanks for the detailed steps! I was able to reproduce the failure when providing a requirements.txt file.

I have reached out to our Legal team to look into allowing you to use, modify, copy, and redistribute this contribution. I will provide updates when I have them.

@jcarey03
Copy link
Contributor Author

jcarey03 commented Jun 8, 2018

@pfifer I just received confirmation on my end that you may use, modify, copy, and redistribute this contribution. Apologies for the delay while we got the legal stuff in order.

@jcarey03
Copy link
Contributor Author

@pfifer, PTAL when you can. Once this merges, when would be the next release of the library?

@pfifer pfifer added this to the v1.5.1 milestone Jun 18, 2018
@pfifer pfifer merged commit e685a37 into awslabs:master Jun 18, 2018
@asottile asottile deleted the exc_on_failed_jar_download branch July 23, 2018 16:33
sahilpalvia added a commit that referenced this pull request Jan 2, 2019
* Updated to version 1.9.3 of the Amazon Kinesis Client Library for Java.
  * #87
* Changed to now download jars from Maven using https.
  * #87
* Changed to raise exception when downloading from Maven fails.
  * #80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants