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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

kaggle argument fix #1101

Merged
merged 14 commits into from Aug 9, 2021
Merged

kaggle argument fix #1101

merged 14 commits into from Aug 9, 2021

Conversation

thisiseshan
Copy link
Contributor

@thisiseshan thisiseshan commented Aug 4, 2021

馃殌 馃殌 Pull Request

Checklist:

  • My code follows the style guidelines of this project and the Contributing document
  • I have commented my code, particularly in hard-to-understand areas
  • I have kept the coverage-rate up
  • I have performed a self-review of my own code and resolved any problems
  • I have checked to ensure there aren't any other open Pull Requests for the same change
  • I have described and made corresponding changes to the relevant documentation
  • New and existing unit tests pass locally with my changes

Changes

add missing kaggle-credentials argument
tiny fix - pls approve

hub/api/dataset.py Outdated Show resolved Hide resolved
@@ -370,6 +371,7 @@ def ingest_kaggle(
- a local file system path of the form ./path/to/dataset or ~/path/to/dataset or path/to/dataset.
- a memory path of the form mem://path/to/dataset which doesn't save the dataset but keeps it in memory instead. Should be used only for testing as it does not persist.
dest_creds (dict): A dictionary containing credentials used to access the destination path of the dataset.
kaggle_credentials (dict): A dictionary containing kaggle credentials {"username":"", "key": ""}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
kaggle_credentials (dict): A dictionary containing kaggle credentials {"username":"", "key": ""}.
kaggle_credentials (dict): A dictionary containing kaggle credentials {"username":"YOUR_USERNAME", "key": "YOUR_KEY"}. If None, environment variables/the kaggle.json file will be used if available.

Copy link
Contributor

@verbose-void verbose-void left a comment

Choose a reason for hiding this comment

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

pls make changes requested but other than that looks good

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #1101 (894b544) into main (78a2f3d) will decrease coverage by 0.11%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1101      +/-   ##
==========================================
- Coverage   90.72%   90.60%   -0.12%     
==========================================
  Files         118      118              
  Lines        6051     6059       +8     
==========================================
  Hits         5490     5490              
- Misses        561      569       +8     
Impacted Files Coverage 螖
hub/api/dataset.py 90.72% <0.00%> (酶)
hub/auto/tests/_test_kaggle.py 0.00% <0.00%> (酶)
hub/auto/unstructured/kaggle.py 22.22% <8.33%> (-0.51%) 猬囷笍
hub/auto/tests/test_ingestion.py 100.00% <100.00%> (酶)

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 78a2f3d...894b544. Read the comment docs.

@thisiseshan
Copy link
Contributor Author

This is a tiny fix, please approve. It fixes a critical bug with ingest_kaggle() API.

@thisiseshan thisiseshan merged commit c88c774 into main Aug 9, 2021
@thisiseshan thisiseshan deleted the small-fix branch August 9, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants