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.

Sign up for GitHub

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

Jump to bottom

kaggle: init at 1.5.6 #92147

Merged
merged 2 commits into from Jul 5, 2020
Merged

kaggle: init at 1.5.6 #92147

merged 2 commits into from Jul 5, 2020

Conversation

cdepillabout
Copy link
Member

Motivation for this change

This adds the kaggle package, which can be used to interact with the API for https://www.kaggle.com/.

Note that if you try to build and run this, you will be presented with an error message like the following:

$ nix-build -A kaggle
$ ./result/bin/kaggle  --help
Traceback (most recent call last):
  File "/nix/store/5hxdnnyg76klxwwfdvh31n92mlkhkvgb-python3.8-kaggle-1.5.6/bin/.kaggle-wrapped", line 6, in <module>
    from kaggle.cli import main
  File "/nix/store/5hxdnnyg76klxwwfdvh31n92mlkhkvgb-python3.8-kaggle-1.5.6/lib/python3.8/site-packages/kaggle/__init__.py", line 23, in <module>
    api.authenticate()
  File "/nix/store/5hxdnnyg76klxwwfdvh31n92mlkhkvgb-python3.8-kaggle-1.5.6/lib/python3.8/site-packages/kaggle/api/kaggle_api_extended.py", line 147, in authenticate
    raise IOError('Could not find {}. Make sure it\'s located in'
OSError: Could not find kaggle.json. Make sure it's located in /home/user/.kaggle. Or use the environment method.

You have to actually create the ~/.kaggle/kaggle.json file as described in the README.md.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@cdepillabout
Copy link
Member Author

@GrahamcOfBorg build kaggle

doronbehar
doronbehar approved these changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Passes nixpkgs-review build and --help test. But certainly that ugly error message you mentioned is brought up unless an api key is located at ~/.kaggle/kaggle.json.

jonringer
jonringer reviewed Jul 4, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

pkgs/development/python-modules/kaggle/default.nix Outdated Show resolved Hide resolved
@cdepillabout @jonringer
kaggle: add simple check to make sure it's not trivially broken
e97b8b6 
Co-authored-by: Jon <jonringer@users.noreply.github.com>
cdepillabout
cdepillabout commented Jul 4, 2020
View reviewed changes
pkgs/development/python-modules/kaggle/default.nix
Comment on lines +39 to +45
checkPhase = ''
export HOME="$TMP"
mkdir -p "$HOME/.kaggle/"
echo '{"username":"foobar","key":"00000000000000000000000000000000"}' > "$HOME/.kaggle/kaggle.json"
$out/bin/kaggle --help > /dev/null
'';
pythonImportsCheck = [ "kaggle" ];
Copy link
Member Author

Choose a reason for hiding this comment

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

@jonringer Thanks for the tip!

Unfortunately, kaggle requires the file ~/.kaggle/kaggle.json to exist, otherwise the CLI throws an exception and ends the process with an error code.

In order to make kaggle --help not end with an error code, I had to create fake api credentials. This is somewhat hacky, but still appears to work for simple things that don't involve actually hitting the API (like just calling --help).

I've confirmed this builds, so if this looks alright, I'd be happy to go ahead and merge this in.

@GrahamcOfBorg build kaggle

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the warning if fine, the $out/bin/kaggle --help still returns 0, even with the warnings.

My main concern is just that a dependency bump map break it, and we won't know about it until runtime (after someone does an update and the package then fails)

@cdepillabout
Copy link
Member Author

Hmm, maybe ofborg didn't see my last command

@GrahamcOfBorg build kaggle

@jonringer
Copy link
Contributor

Hmm, maybe ofborg didn't see my last command

not sure if ofborg see's reviews, it may only apply to comments

@cdepillabout cdepillabout merged commit f233572 into NixOS:master Jul 5, 2020
@cdepillabout cdepillabout deleted the add-kaggle branch July 5, 2020 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@doronbehar doronbehar doronbehar approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cdepillabout @jonringer @doronbehar