Skip to content

Conversation

@galipremsagar
Copy link
Contributor

This is a follow-up PR to address review comments from here: #10769 (review)

This PR:

  • Uses ThreadedMotoServer instead of using subprocess.open to create a new server, this way it is guaranteed to close the server upon exit.
  • Add's IP address fixture instead of having it hard-coded at multiple places.

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 10, 2022
@galipremsagar galipremsagar requested a review from bdice May 10, 2022 18:43
@galipremsagar galipremsagar requested a review from a team as a code owner May 10, 2022 18:43
@galipremsagar galipremsagar self-assigned this May 10, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner May 10, 2022 18:43
@galipremsagar galipremsagar requested a review from shwina May 10, 2022 18:43
@galipremsagar galipremsagar changed the title [REVIEW] Use ThreadedMotoServer instead of subprocess [REVIEW] Use ThreadedMotoServer instead of subprocess in spinning up s3 server May 10, 2022
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

One suggestion for cleaning up the import-or-skip behavior. Otherwise LGTM! Thanks for these improvements, @galipremsagar!

@galipremsagar galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label May 10, 2022
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #10822 (ef0d792) into branch-22.06 (8d861ce) will decrease coverage by 0.09%.
The diff coverage is 94.89%.

❗ Current head ef0d792 differs from pull request most recent head 1df1fe3. Consider uploading reports for the commit 1df1fe3 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06   #10822      +/-   ##
================================================
- Coverage         86.40%   86.31%   -0.10%     
================================================
  Files               143      144       +1     
  Lines             22448    22640     +192     
================================================
+ Hits              19396    19541     +145     
- Misses             3052     3099      +47     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/lists.py 91.66% <ø> (-0.42%) ⬇️
python/cudf/cudf/core/column/string.py 88.78% <ø> (-0.31%) ⬇️
python/cudf/cudf/core/frame.py 93.41% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py 91.70% <ø> (ø)
python/cudf/cudf/io/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/io/parquet.py 90.83% <86.60%> (-1.87%) ⬇️
python/cudf/cudf/core/index.py 92.06% <88.88%> (-0.25%) ⬇️
python/cudf/cudf/core/scalar.py 89.01% <90.90%> (-0.31%) ⬇️
python/cudf/cudf/core/dataframe.py 93.77% <96.29%> (+0.08%) ⬆️
python/cudf/cudf/__init__.py 90.69% <100.00%> (+0.22%) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fcd364...1df1fe3. Read the comment docs.

@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit dc0c3cd into rapidsai:branch-22.06 May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants