Skip to content

General code cleanup part #1#29

Merged
parente merged 9 commits intoadtech-labs:masterfrom
parente:general-cleanup
May 10, 2017
Merged

General code cleanup part #1#29
parente merged 9 commits intoadtech-labs:masterfrom
parente:general-cleanup

Conversation

@parente
Copy link
Contributor

@parente parente commented May 10, 2017

  • Adding docstrings everywhere
  • Simplifying state tracking in a few place
  • Removing dead code
  • Addressing minor output problems

Prelude to #28, 26, and #22 which will build atop this but that
I want to keep separate for easier review.

parente added 7 commits May 8, 2017 23:04
Move usage of coverage to run_tests
* spylon can already do this
* It was broken looking for the python cell magic
* It uses a metakernel.IPythonKernel instead of the active IPython kernel
* Show a reasonable error message when input is 
  incomplete according to the interpreter
* Only use one executors in the thread pool: we
  must do one thing at a time anyway
Going to do this separately after the general cleanup

This reverts commit c8fa833.
@codecov
Copy link

codecov bot commented May 10, 2017

Codecov Report

Merging #29 into master will decrease coverage by 1.17%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   85.48%   84.31%   -1.18%     
==========================================
  Files           6        6              
  Lines         434      408      -26     
==========================================
- Hits          371      344      -27     
- Misses         63       64       +1

@parente
Copy link
Contributor Author

parente commented May 10, 2017

Whoops. Gotta put codecov back in.

Copy link
Collaborator

@mariusvniekerk mariusvniekerk left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this

%%init_spark
application_name = "My Fancy App"
launcher.jars = ["file://some/jar.jar"]
launcher.master = "local[4]"

Choose a reason for hiding this comment

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

What in the world is this local[4] syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spark syntax for how many cores to use. local[4] says "Please use 4 of my cores for the application master." local[*] says "TAKE ALL MY CORES PLZ!"

Choose a reason for hiding this comment

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

Can you add that as a comment in the code?

Copy link

@ericdill ericdill left a comment

Choose a reason for hiding this comment

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

LGTM? I know very little about this codebase but nothing is standing out to me

@parente
Copy link
Contributor Author

parente commented May 10, 2017

Going to merge what I've got, then carry on with documenting the other two modules in another PR. Thanks folks.

@parente parente changed the title [WIP] General code cleanup General code cleanup part #1 May 10, 2017
@parente parente merged commit 07d4ee2 into adtech-labs:master May 10, 2017
@parente parente mentioned this pull request May 11, 2017
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.

3 participants