BuildManager #28

mpacer · 2018-03-15T03:04:59Z

I want to make the build procedure more isolated chunks that have specific purposes so that they could A: be tested separately and B: understood separately. I think there are also a number of instances where we're issuing shell commands that we could do from within python. (edited)

This is the first step in refactoring the actual builder.

Right now, this is just a WIP because for some reason despite using the
same code, it seems to be pulling in the entire upstream repo (including
all of its papers) rather than just the proceedings build tools. I don't
know why it works for the standard build script but not the main build
script.

I'm stopping for now because I think I overran my github rate limit for the next couple of hours.

stefanv · 2018-03-15T05:41:48Z

procbuild/builder.py

@@ -10,7 +10,7 @@
 import random

 from os.path import join as joinp
-from glob import glob
+from glob import glob, iglob


glob no longer used

Once i remove the old code, indeed

stefanv · 2018-03-15T05:50:29Z

procbuild/builder.py

+
+class BuildManager(object):
+
+    def __init__(self, 


odd formatting here

it makes it easier to change the variables without introducing confusing diffs since I didn't expect these to stay constant. I actually like this format if we end up having a lot of keyword arguments (which I think a lot of these are going to become), for similar reasons.

I'm also thinking that we might want to introduce a "type" system where we create an object first and then can check that everything is well formatted and pass in that instance to the BuildManager.

The simpler we can keep things the better. The words "type system" fill me with fear!

i actually was putting quotes only around "type" to distinguish it from "type system". Basically I want one thing that does all the preamble to defining what the builder will need to know, instantiate that object (and make it debugable) and then pass that into the BuildManager which will do the actual build management. Right now the BuildManager is playing both roles.

stefanv · 2018-03-15T05:51:03Z

procbuild/builder.py

+    def build_paper(self):
+        try:
+            with tempfile.TemporaryDirectory() as build_path:
+                self.build_path = build_path


This section reads very nicely now. Would it make sense to change some of the methods to private members, e.g. self._run_make_paper_script. My thinking is that we should signal to the user that build_paper is really what they want to be calling.

that seems reasonable, what I'm hoping is that this will also make it substantially more testable.

stefanv · 2018-03-15T05:52:41Z

procbuild/server.py

@@ -197,6 +197,8 @@ def _build_worker(nr):

    def build_and_log(*args, **kwargs):


*args, **kwargs is such an impenetrable syntax. It would be helpful to document either this method of BuildManager.build_paper.

Agreed, I actually think this is an unnecessary framework that will go away once we're switching to zmq for messaging.

I apologise if I was unclear, I was suggesting that we do not change this syntax in this PR. Rather, we wait to change this syntax in a later PR (e.g., #29).

stefanv · 2018-03-15T05:53:24Z

procbuild/builder.py

+        self.build_pdf_path = ''
+        self.master_repo_path = joinp(cache(), 'scipy_proceedings')
+        self.build_timestamp = time.strftime('%d/%m %H:%M')
+        self.target_path = joinp(cache(), '%s.pdf' % target)


Should this be an input parameter as well?

stefanv · 2018-03-15T05:54:29Z

procbuild/builder.py

+        self.master_branch = master_branch
+        self.build_status = 'Build started...'
+        self.build_pdf_path = ''
+        self.master_repo_path = joinp(cache(), 'scipy_proceedings')


Should this be an input parameter as well?

(my thinking is that the BuildManager should not be dependent on knowledge about the cache)

Agreed on all of these counts. I also have a PRManager PR that i'm working on that changes how the cache works, I should probably make a WIP PR for that too… I just think it'll be easier to get these refactors done one at a time & this one was more isolated.

NB: that comment was relevant because the cache should move into the PRManager since that's what's doing most of the cache management.

mpacer · 2018-03-18T01:52:22Z

@stefanv i made the change you suggested, but ~~it's not actually stopping the other papers from being drawn in… i'm not sure why~~ it required that i first realise that i was already inside the scipy_proceedings directory when deleting the papers.

mpacer · 2018-03-18T05:45:58Z

Huzzah, now it works!

stefanv

Some minor questions / comments, but overall looks very good. Thanks @mpacer!

stefanv · 2018-03-19T21:51:31Z

procbuild/builder.py

+        self.add_output(output)
+
+        if errcode:
+            self.add_output('[X] Error code %d ' % errcode)


[X] Clean out tools papers directory or something similar?

Ok — I was trying to directly map it onto what was being done before as a simple fix.

stefanv · 2018-03-19T21:51:48Z

procbuild/builder.py

+        self.add_output(output)
+
+        if errcode:
+            self.add_output('[X] Error code %d\n' % errcode)


Same here: try to make the error descriptive.

stefanv · 2018-03-19T21:52:31Z

procbuild/builder.py

+        return joinp(self.build_path, 'papers', self.paper)
+
+    @property
+    def paper(self):


What does this method do? Naming doesn't guide the reader, and no docstring.

I see now how this works. Hrm, wondering if it makes sense to have this at all, or whether it could just be a parameter to the build_paper method?

Making it a parameter would require that we do the parsing of the PR outside of the Build manager, right now all of that parsing is being done in here. If we were going to switch that, I would want to include that functionality in the PRManager, which is a bigger refactor. Is this fine for now?

In the meantime I can add a docstring

stefanv · 2018-03-19T21:52:57Z

procbuild/builder.py

+            return papers[0].split('/')[-1]
+
+    def _run_make_paper_script(self):
+        """Runs the scipy_proceedings make_paper.sh script


Isn't this script just a wrapper around the build manager?

No, this actually builds the paper using the make_paper.sh script from scipy_proceedings.

I tried to write that in the docstring for that method… can you suggest how it could be made clearer?

Right now, this is just a WIP because for some reason despite using the same code, it seems to be pulling in the entire upstream repo (including all of its papers) rather than just the proceedings build tools. I don't know why it works for the standard build script but not the main build script.

@stefanv

thx @stefanv for the idea

also update some strings to be f-strings

stefanv reviewed Mar 15, 2018

View reviewed changes

mpacer changed the title ~~WIP: BuildManager~~ BuildManager Mar 18, 2018

mpacer mentioned this pull request Mar 18, 2018

Use ZeroMQ message bus in lieu of build queue using new BuildManager #29

Merged

stefanv reviewed Mar 19, 2018

View reviewed changes

mpacer added 9 commits March 19, 2018 16:01

complete builder by removing papers dir from build_tools repo

c25ea5a

thx @stefanv for the idea

make methods private to discourage use directly

37022aa

Code now works! Fix path issues, remove old build function

d4ae9af

also update some strings to be f-strings

change BuildManager to expect cache as input parameter

015d809

improve error messages in build condition failures on @stefanv's request

36e5677

add docstring to self.paper

97ce298

Add docstring to Build Manager

6d6259c

remove glob from import

07db9d3

mpacer force-pushed the build_manager branch from a4397ee to 07db9d3 Compare March 19, 2018 23:01

mpacer added 3 commits March 19, 2018 16:37

move top level varaibles into the __init__.py

0b7ff7a

move utilities into utils module

dd21378

move pr related utilities into the pr_list subpackage

5f04b0a

stefanv merged commit c1f1215 into scipy-conference:master Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BuildManager #28

BuildManager #28

mpacer commented Mar 15, 2018 •

edited

Loading

stefanv Mar 15, 2018

mpacer Mar 18, 2018

stefanv Mar 15, 2018

mpacer Mar 15, 2018

stefanv Mar 15, 2018

mpacer Mar 19, 2018

stefanv Mar 15, 2018

mpacer Mar 15, 2018

stefanv Mar 15, 2018

mpacer Mar 15, 2018

mpacer Mar 20, 2018

stefanv Mar 15, 2018

mpacer Mar 15, 2018

stefanv Mar 15, 2018

stefanv Mar 15, 2018

mpacer Mar 15, 2018

mpacer Mar 15, 2018

mpacer commented Mar 18, 2018 •

edited

Loading

mpacer commented Mar 18, 2018

stefanv left a comment

stefanv Mar 19, 2018

mpacer Mar 19, 2018

stefanv Mar 19, 2018

stefanv Mar 19, 2018

stefanv Mar 19, 2018

mpacer Mar 19, 2018

mpacer Mar 19, 2018

stefanv Mar 19, 2018

mpacer Mar 19, 2018 •

edited

Loading

mpacer Mar 19, 2018

		@@ -197,6 +197,8 @@ def _build_worker(nr):

		def build_and_log(args, *kwargs):

BuildManager #28

BuildManager #28

Conversation

mpacer commented Mar 15, 2018 • edited Loading

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

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

mpacer commented Mar 18, 2018 • edited Loading

mpacer commented Mar 18, 2018

stefanv 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

mpacer Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpacer commented Mar 15, 2018 •

edited

Loading

mpacer commented Mar 18, 2018 •

edited

Loading

mpacer Mar 19, 2018 •

edited

Loading