Run examples with separate processes #314

devmotion · 2021-06-28T13:28:31Z

This is an (arguably) simpler alternative to #309 and (partially) #303.

Examples are run asynchronously with different processes and no load path trickery is needed. CompatHelper is updated such that example environment are updated as well. I also updated and fixed the SVM example to demonstrate that the pipeline works as expected (the other examples are not blacklisted but since they are not moved to a separate folder they are not run currently).

Edit: I did not base this PR on #303 intentionally since in this way there are no changes hidden in the other PR and no major unrelated rewriting is needed. The PR is self-contained and as minimal as possible. I assumed that the other examples would be fixed in separate PRs, e.g., by rebasing #303.

examples/svm/script.jl

devmotion · 2021-06-28T16:13:07Z

This should be good to go.

Documenter: https://juliagaussianprocesses.github.io/KernelFunctions.jl/previews/PR314/examples/svm/
Notebook: https://nbviewer.jupyter.org/github/JuliaGaussianProcesses/KernelFunctions.jl/blob/gh-pages/previews/PR314/examples/svm.ipynb

willtebbutt

Some minor comments. Would be helpful for others to review as well, as they've spent more time looking at this than I.

docs/make.jl

examples/svm/script.jl

willtebbutt · 2021-06-28T16:41:54Z

examples/svm/script.jl

+
+# Optimal prediction:
+function f(x, X, k, λ)
+    return kernelmatrix(k, x, X) / (kernelmatrix(k, X) + exp(λ) * I) * y


Also, is this not just kernel ridge regression, rather than support vector regression? Possibly I'm forgetting something basic here, because I've not looked at SVM stuff for many years...

Haha yes I had the exact same comment on Slack 😅

Haha yeah I just copied and fixed the existing example to show that the setup works without thinking about the content at all 😄 Maybe we should just remove the example?

That's why we we switched from SVM to kernel ridge regression on the other PR

This "SVM" example is also part of #303 (which is less optimized/updated than the version here). There exists another kernel ridge regression and a deep kernel learning example that are also updated in #303.

Therefore my suggestion would be to just completely remove the SVM example (and also not to keep the current version). The current version showed that the setup works and other examples can then be updated in separate PRs.

🤷‍♀️ happy to delete it as well and only put it back once we've done it properly. I would just not get too hung up on the contents of the example notebooks, let's get the doc build in order, and then we can rescue the actual content from #234

I moved the updated version back to examples/svm.jl since it is still an improvement even though it's not actually SVM 😄

st-- · 2021-06-29T06:18:29Z

.github/workflows/CompatHelper.yml

+      - name: "Run CompatHelper"
+        run: |
+          import CompatHelper
+          subdirs = ["", "test", "docs"]


What does the empty "" do here? If it's referring to the repo base directory, why would it need to be passed to _sub_dirs?

Otherwise the base directory is not updated anymore. The default setting is subdirs = [""]: https://juliaregistries.github.io/CompatHelper.jl/dev/options/#Packages-in-subdirectories

st-- · 2021-06-29T06:19:18Z

.github/workflows/CompatHelper.yml

+        run: |
+          import Pkg
+          name = "CompatHelper"
+          uuid = "aa819f21-2bde-4658-8897-bab36330d9b7"


Why do you want to change to pinning CompatHelper? is there any reason not to want the latest version (with potential bugfixes etc)?

The UUID is just an additional guarantee that it is the correct package but does not fix the version. The version is only pinned to 2.x.x but no specific version, i.e., it has to be updated only if there is a breaking release of CompatHelper.

In general, this is just copied from the documentation of CompatHelper: https://juliaregistries.github.io/CompatHelper.jl/dev/

.github/workflows/docs.yml

st-- · 2021-06-29T06:24:43Z

docs/make.jl

+
+# Check that all examples were run successfully
+if !all(success, processes)
+    error("some examples were not run successfully")


Would there be any way of indicating which script failed ?

I redirected stderr of the processes to the default stderr, and hence errors of the individual processes are printed in the terminal regardless of success. The check here mainly ensures that all processes are done and that the script errors if a process failed.

st-- · 2021-06-29T06:25:08Z

docs/make.jl

+            map(filter!(filename -> endswith(filename, ".md"), readdir(EXAMPLES_OUT))) do x
+                return joinpath("examples", x)
+            end,


nice:) much easier to read than the other version

st-- · 2021-06-29T07:26:16Z

docs/literate.jl

+
+# Activate environment
+using Pkg: Pkg
+Pkg.activate(dirname(SCRIPTJL))


Requiring the examples scripts to include Literate in their dependencies when they don't actually use it seems weird; maybe that's the best we can do (and of course we can then clarify that in the examples/README.md), but why not add them here as well:

Suggested change

Pkg.activate(dirname(SCRIPTJL))

Pkg.activate(dirname(SCRIPTJL))

Pkg.add("Literate")

this should add it to the correct Project.toml (e.g. when I write a new examples script and locally run make.jl), but be a no-op if it's already there.

I thought about this as well and I am still not sure what's the best approach. The advantage of adding them to the Project.toml + Manifest.toml is that it is correctly tracked and updated by CompatHelper and that it is guaranteed that the installation of Literate does not downgrade any other packages or mess with the package setup which would impact reproducibility if it is not part of Project.toml + Manifest.toml (otherwise it might only be installed when the CI tests are run).

Would

Suggested change

Pkg.activate(dirname(SCRIPTJL))

Pkg.activate(dirname(SCRIPTJL))

Pkg.add("Literate"; preserve=PRESERVE_ALL)

or similar (Pkg.add docs) not satisfy that?

Co-authored-by: st-- <st--@users.noreply.github.com>

willtebbutt

This looks good to me now.

Perhaps renaming the svm file to kernel_ridge_regression.jl or something would make sense while we're here though 🤷

edit: sorry, just understood what you're saying about the other example of ridge regression. Perhaps just replace that with this one? As you say, it's much cleaner.

st--

Looks good to me; I would still move the Literate dependency from examples scripts themselves into the literate.jl script, but happy to propose that change in #303 after rebasing and continuing the discussion there:)

Following #314, this PR adds READMEs for how to build & add to the docs and examples, as well as tidying the workflow up a bit more. It makes all current examples run (with a warning to say they're under construction).

devmotion added 3 commits June 28, 2021 15:22

Run examples with separate processes

cd068bf

Update CompatHelper

c3956dc

Fix SVM example

dbaa9b3

github-actions bot reviewed Jun 28, 2021

View reviewed changes

examples/svm/script.jl Outdated Show resolved Hide resolved

devmotion added 5 commits June 28, 2021 15:39

Fix GR error messages

1b4e9fa

Improve SVM example

5783d9f

Try to fix edit link

efa6acf

Fix preprocessing

e5e4dfe

Fix links

f863314

devmotion requested review from theogf, willtebbutt and st-- June 28, 2021 16:18

willtebbutt reviewed Jun 28, 2021

View reviewed changes

st-- reviewed Jun 29, 2021

View reviewed changes

.github/workflows/docs.yml Outdated Show resolved Hide resolved

st-- reviewed Jun 29, 2021

View reviewed changes

devmotion and others added 4 commits June 29, 2021 10:36

Move "SVM" example back to svm.jl

ce3037f

Minor simplification

fca865c

Update .github/workflows/docs.yml

cdb955c

Co-authored-by: st-- <st--@users.noreply.github.com>

Fix test errors

abef80d

devmotion requested review from st-- and willtebbutt June 29, 2021 08:46

willtebbutt approved these changes Jun 29, 2021

View reviewed changes

st-- approved these changes Jun 29, 2021

View reviewed changes

st-- merged commit 69ea829 into master Jun 29, 2021

st-- deleted the dw/examples branch June 29, 2021 09:42

st-- mentioned this pull request Jun 29, 2021

Use processes to run literate examples #309

Closed

devmotion mentioned this pull request Jun 29, 2021

Examples in the docs: base PR for cleaning up notebook workflow #303

Merged

st-- mentioned this pull request Jun 30, 2021

Example notebooks #234

Closed

	Pkg.activate(dirname(SCRIPTJL))
	Pkg.activate(dirname(SCRIPTJL))
	Pkg.add("Literate")

	Pkg.activate(dirname(SCRIPTJL))
	Pkg.activate(dirname(SCRIPTJL))
	Pkg.add("Literate"; preserve=PRESERVE_ALL)

Run examples with separate processes #314

Run examples with separate processes #314

Uh oh!

Conversation

devmotion commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

devmotion commented Jun 28, 2021

Uh oh!

willtebbutt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willtebbutt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

st-- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devmotion commented Jun 28, 2021 •

edited

Loading

willtebbutt left a comment •

edited

Loading