Updated CircleCI configuration #116
Conversation
…r each notebook. Also added TestCase handling that'll report when errors are found with the pip install -r requirements.txt.
There was a problem hiding this comment.
I few inline comments, but here I'm leaving some bigger-picture ones mostly about where this code is exposed rather than the implementation itself (which looks pretty much good to me):
- If I'm understanding right, this PR is now bypassing the
convert.pyfile. This is problematic because the intent is that the user who wants to reproduce what's happening on circleci should be able to runconvert.pyand get the exact same thing. Moving that implementation tocreate_artifacts.pythen makes it likely that the CircleCI andconvert.pyinterfaces will diverge to the point that they are completely separate, which is likely to both confuse users and create a bigger maintainence burden. So is there perhaps a way to changecreate_artifacts.pyto call the "standard"convert.pyprocess (which can be changed in whatever way is necessary) while adding the artifact-storing as a second stage? Or alternatively,convert.pycan be turned into a "command line interface" to what's in the other file (although see my next comment on that). - More broadly, some of this machinery is not really specific to CircleCI. That means it should go in
nbpagesitself rather than in the.circlecidirectory. A viable pathway might be to put this code somewhere temporarily (perhaps even in.circlecias it is right now) to prove that it works, and then port it over tonbpagesas a follow-on PR (with a corresponding separate "remove it from this repo" PR)
.circleci/build_artifacts.py
Outdated
| if filename.endswith('.tar.gz'): | ||
| yield os.path.join(start_dir, filename) | ||
|
|
||
| for artifact_path in find_artifacts(ARTIFACT_DEST_DIR): |
There was a problem hiding this comment.
Probably the below should be an if __name__ == '__main__' block? Not critical, but might be useful for porting to nbpages (see general comment)
There was a problem hiding this comment.
Or alternatively, make this a main function, and have an if __name__ == '__main__' that does command-line parsing.
.circleci/build_artifacts.py
Outdated
| xml.add_testsuite(test_suite) | ||
| xml.write(test_output_path) | ||
|
|
||
| # from nbpages import make_parser, run_parsed, make_html_index |
There was a problem hiding this comment.
Is this left-over from a previous attempt, or is it intentional that there's a commented-out part here? (and if the latter, maybe add a comment above explaining why)
There was a problem hiding this comment.
this logic was moved to make_pages.py; I failed to remove the comments; fixed
| - webbpsf>=0.8 | ||
| - scikit-learn | ||
|
|
||
| - astropy=3.0.4 |
There was a problem hiding this comment.
Why the shift to so many things in pip instead of conda? One of the intentions here is to make sure the astroconda-installed version works as intended... pip is meant for only if something is not in astroconda.
There was a problem hiding this comment.
Ah, I think the answer is for performance. Possibly a good idea then, but I think this should be done as a separate independent PR because it has other consequences. So I'd say just don't make any changes to the environment beyond what might be needed to support the new CCI machinery, and move these changes to a separate PR
|
hey @eteq , I've updated the I'm okay with moving some of this machinery into |
There was a problem hiding this comment.
(One minor inline suggestion, which is not a dealbreaker or anything if I mis-understood the intent)
@jbcurtin - ah, right - I meant we would port this circleci configuration into nbpages. Right now nbpages doesn't have "baked in" CI, but it would be useful to have it in the templates (e.g. https://github.com/eteq/nbpages/tree/cookiecutter_html).
Also, I think the artifact-building might also be useful to put in nbpages - while some of the details are circle-specific, the general idea there is useful for other CI and/or manual-pushing.
Regardless, I agree it makes sense to use this as a "testing ground" for now. Once we're convinced this works well here we could push it upstream.
Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>
|
hey @eteq , I've added your suggestion and the build succeeds. Will you merge the codes here? |
Updated the Circle CI Integration to compile each notebook individually before building all the pages. Failures are reported when the notebook cannot be built for some reason.
With this update, CircleCI was also update for
spacetelescope/notebooksBuilds will only happen on
master, every PR branch, and every forked PR branchHere is a view of the CircleCI with reporting enabled
Opened a new PR with better merge vector for commit history. ( Old PR: #115)