Add docstrings and minor style fixes for application files and JuliaBuildPack #213

willingc · 2018-02-05T17:59:20Z

Partially addresses #211

Covers repo2docker application files:

app.py
utils.py
__main__.py

And JuliaBuildPack

yuvipanda

This is awesome, @willingc! Left some comments with suggested changes, but everything looks pretty accurate to me.

yuvipanda · 2018-02-06T00:58:25Z

repo2docker/app.py

    name = 'jupyter-repo2docker'
    version = __version__
    description = __doc__

    @default('log_level')
    def _default_log_level(self):
+        """The system's default log level"""


The application's default log level?

yuvipanda · 2018-02-06T01:02:19Z

repo2docker/app.py

-        Validate image_name read by argparse contains only lowercase characters
+        Validate image_name read by argparse
+
+        Note: image_name contains only lowercase characters


Ah, I think you've found a bug in our docs :) This is not the actual validation logic we use nor the actual liimts, but I see our exception in 194 says this too.

The logic we use + the valid criteria is 'Container names must start with an alphanumeric character and can then use _ . or - in addition to alphanumeric. [a-zA-Z0-9][a-zA-Z0-9_.-]+'. We can either update that in this PR or do that in a separate one! Good catch!

yuvipanda · 2018-02-06T22:54:27Z

repo2docker/app.py

@@ -444,6 +463,7 @@ def push_image(self):
                last_emit_time = time.time()

    def run_image(self):
+        """Run docker image"""


"Run docker container from built image" perhaps?

yuvipanda · 2018-02-06T22:59:50Z

repo2docker/buildpacks/julia.py

    """
    def get_env(self):
+        """Get additional environment settings for Julia and Jupyter
+
+        Returns:


I think the docstrings here should explain what exactly is being returned, since the generic docstring for what get_env is returning is already in the base BuildPack. I think explaining each of the env variable set here and why we do it would be the useful thing here.

JULIA_PATH - base path where all Julia Binaries and libraries will be installed

JULIA_HOME - path where all Julia Binaries will be installed

JULIA_PKGDIR - path where all Julia libraries will be installed

JULIA_VERSION - default version of julia to be installed

JUPYTER - environment variable required by IJulia to point to jupyter executable

yuvipanda · 2018-02-06T23:00:38Z

repo2docker/buildpacks/julia.py

@@ -22,9 +31,30 @@ def get_env(self):
        ]

    def get_path(self):
+        """Get path for Julia executables
+
+         Returns:


Adds path to Julia binaries to user's PATH.

Also we can simplify this code to just be:

return super().get_path() + ['${JULIA_HOME}']

but also ok to not do that!

yuvipanda · 2018-02-06T23:01:39Z

repo2docker/buildpacks/julia.py

+
+        This sets up:
+
+        - a directory for the specified Julia version and path


It installs Julia itself, and also installs IJulia (Pkg.add sounds like you are adding to something, but it really is installing the package)

yuvipanda · 2018-02-06T23:01:50Z

repo2docker/buildpacks/julia.py

+        """
+        Return series of build-steps specific to "this" Julia repository
+
+        Precompile all Julia libraries found in the repository's REQUIRE


yuvipanda · 2018-02-06T23:02:49Z

repo2docker/buildpacks/julia.py

+        """
+        Check if current repo should be built with the Julia Build pack
+
+        super().detect() is called if Julia Build pack should be used.


I think similar to the R build pack, we should actually not call super().detect() here, for the same reason as on the R build pack (we want to use Julia even if there is no environment.yml file). Whenever I'm doing doc fixes I'm also happy to fix bugs so documenting them is easier, but happy to deal with that in a separate PR too!

yuvipanda · 2018-02-06T23:03:23Z

repo2docker/utils.py

@@ -53,55 +54,65 @@ def flush():

 @contextmanager
 def maybe_cleanup(path, cleanup=False):
+    """Execute cleanup of the passed path when cleanup flag is True."""


"Delete the directory at passed path if cleanup flag is True"

yuvipanda · 2018-02-07T19:24:33Z

repo2docker/buildpacks/julia.py

+        This installs:
+
+        - the specified Julia version and path
+        - Julia packages with ownership granted to the notebook user


It only creates a directory with permissions for installing julia packages (from get_assemble_scripts), does not actually install them here

Thanks. I will update. Still working on the corrections from yesterday.

willingc · 2018-03-15T19:38:51Z

@yuvipanda I think that I've covered all your review comments.

minrk · 2018-03-19T17:21:57Z

Great!

willingc · 2018-03-19T17:39:19Z

Thanks @minrk for the merge. Going to try to finish all the docstrings today.

Add docstrings and minor style fixes for application files and JuliaBuildPack

yuvipanda mentioned this pull request Feb 5, 2018

Write detailed docstrings for all current BuildPacks #211

Closed

6 tasks

willingc changed the title ~~[WIP] Add docstrings and minor style fixes~~ [WIP] Add docstrings and minor style fixes for application files and JuliaBuildPack Feb 5, 2018

willingc force-pushed the docstring-conda branch from e2948bb to cd78015 Compare February 5, 2018 21:27

willingc changed the title ~~[WIP] Add docstrings and minor style fixes for application files and JuliaBuildPack~~ Add docstrings and minor style fixes for application files and JuliaBuildPack Feb 5, 2018

yuvipanda requested changes Feb 6, 2018

View reviewed changes

willingc force-pushed the docstring-conda branch from 109512c to d627200 Compare February 7, 2018 01:29

yuvipanda reviewed Feb 7, 2018

View reviewed changes

yuvipanda mentioned this pull request Feb 8, 2018

Improve Julia handling so an environment.yml is not required #103

Closed

willingc added this to the 0.6 milestone Mar 13, 2018

willingc added 11 commits March 15, 2018 12:12

organize imports

9b25188

add docstrings for juliabuildpack

5b5c6c2

add docstrings and clarify difference between build and assemble

e428ddd

minor edit

e85918b

add docstrings and format regex

c7bb205

add docstrings and minor style edits

5b73968

add yuvipanda text suggestions

c231531

add review suggestion

a6e02c8

update julia docstrings per review

4db31cb

fix typo

273110e

refactor detect() to not call super

fc3e370

willingc force-pushed the docstring-conda branch from 9623fc0 to fc3e370 Compare March 15, 2018 19:16

willingc added 2 commits March 15, 2018 12:28

update validation logic error message and docstring

245e7d1

expand julia docstrings

8a3f79f

update tests for image name validation

2c60b0f

minrk merged commit 0567187 into jupyterhub:master Mar 19, 2018

willingc deleted the docstring-conda branch March 19, 2018 17:39

markmo pushed a commit to markmo/repo2docker that referenced this pull request Jan 22, 2021

Merge pull request jupyterhub#213 from willingc/docstring-conda

7fc0a12

Add docstrings and minor style fixes for application files and JuliaBuildPack

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstrings and minor style fixes for application files and JuliaBuildPack #213

Add docstrings and minor style fixes for application files and JuliaBuildPack #213

willingc commented Feb 5, 2018 •

edited

Loading

yuvipanda left a comment

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

willingc Mar 15, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 6, 2018

yuvipanda Feb 7, 2018

willingc Feb 7, 2018

willingc commented Mar 15, 2018

minrk commented Mar 19, 2018

willingc commented Mar 19, 2018


		This sets up:

		- a directory for the specified Julia version and path

Add docstrings and minor style fixes for application files and JuliaBuildPack #213

Add docstrings and minor style fixes for application files and JuliaBuildPack #213

Conversation

willingc commented Feb 5, 2018 • edited Loading

yuvipanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc commented Mar 15, 2018

minrk commented Mar 19, 2018

willingc commented Mar 19, 2018

willingc commented Feb 5, 2018 •

edited

Loading