Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tutorial bugfixes and CI integration #472

Merged
merged 13 commits into from
Feb 1, 2021

Conversation

srush
Copy link
Contributor

@srush srush commented Jan 20, 2021

There were some a couple remaining issues with the tutorial due to recent changes and conflicts in edits. This fixes them and runs on the latest version.

@google-cla google-cla bot added the cla: yes label Jan 20, 2021
@oxinabox
Copy link
Contributor

oxinabox commented Jan 20, 2021

Can we add the tutorial back to the CI? so we can be sure not to break it in the future?
It should be just adding it here:

dex-lang/makefile

Lines 88 to 92 in d178dc0

example-names = mandelbrot pi sierpinski rejection-sampler \
regression brownian_motion particle-swarm-optimizer \
ode-integrator mcmc ctc raytrace particle-filter \
isomorphisms ode-integrator linear_algebra fluidsim \
sgd chol

Copy link
Collaborator

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Thanks for further cleaning up the tutorial!

Comment on lines +462 to +465
interface Add a
add : a -> a -> a
sub : a -> a -> a
zero : a
Copy link
Collaborator

@dan-zheng dan-zheng Jan 20, 2021

Choose a reason for hiding this comment

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

Could you please briefly explain why the trailing ' can be dropped from the declaration names here?

I tried dropping the trailing ' from names a few days ago and got "multiple definition" errors due to conflicts with Prelude declarations. Maybe that's not an issue anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the Multiple Definitions error was coming from the type of tableMean

def tableMean' [Add v, VSpace v] (x : n => v) : v =

which I changed to

def tableMean' [VSpace v] (x : n => v) : v =

VSpace implies Add. But not sure why that breaks it.

Also I think you add the scaling in VSpace reversed (it's mul not div right?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure re-defining a type class should also error. Can you please add this example to the makefile tests so that we can check if it succeeds in CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Needed to download the files in the CI. hopefully that is okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm this is a good question... I honestly don't know whether we can do that or not. The worst part is that the MNIST download page doesn't even mention a license, so I'm a bit reluctant to do so 😕 Could we use a different dataset by any chance?

examples/tutorial.dx Show resolved Hide resolved
@srush
Copy link
Contributor Author

srush commented Jan 20, 2021

@oxinabox Can you briefly explain how CI works? How do I indicate that lines in the tutorial should be errors on purpose?

@dan-zheng
Copy link
Collaborator

dan-zheng commented Jan 20, 2021

Can we add the tutorial back to the CI? so we can be sure not to break it in the future?

Does the tutorial require locally downloading MNIST files? Seems like it would break easily on CI machines, unless there's some Dex dataset downloading logic within Dex itself (getOrCloneMNISTData). I would be really happy to see that!

Can you briefly explain how CI works? How do I indicate that lines in the tutorial should be errors on purpose?

As @oxinabox kindly noted, simply add tutorial to example-names in makefile. Then, you can run tests (including testing examples/tutorial.dx) via make tests.

dex-lang/makefile

Lines 88 to 92 in d178dc0

example-names = mandelbrot pi sierpinski rejection-sampler \
regression brownian_motion particle-swarm-optimizer \
ode-integrator mcmc ctc raytrace particle-filter \
isomorphisms ode-integrator linear_algebra fluidsim \
sgd chol

@srush
Copy link
Contributor Author

srush commented Jan 20, 2021

My assumption that the CI would fail on an error in the code. How do I indicate in Dex that certain errors are expected?

@dan-zheng
Copy link
Collaborator

dan-zheng commented Jan 20, 2021

My assumption that the CI would fail on an error in the code. How do I indicate in Dex that certain errors are expected?

Ah, good question. I think dex script foo.dx is designed to produce a quine: you can save the output to a file, and re-running dex script on that file should produce itself as output.

I think this "expected output" checking may apply not only to successful code blocks (e.g. :p value) but also errors? You can individually test examples/tutorial.dx by running make run-examples/tutorial - the Makefile has some neat logic for supporting that.

Hope this helps, let us know if you have further specific questions!

@srush
Copy link
Contributor Author

srush commented Jan 20, 2021

Thanks! I'll do that in another PR. One last question: Is there a command like dex script to build the static evaluated html (as opposed to the server)?

@oxinabox
Copy link
Contributor

I have no answered @dan-zheng has not given better.

@srush
Copy link
Contributor Author

srush commented Jan 20, 2021

Neat, this passes now using the quine thing. That's really cute, I had no idea those lines were tests.

@dan-zheng
Copy link
Collaborator

dan-zheng commented Jan 20, 2021

One last question: Is there a command like dex script to build the static evaluated html (as opposed to the server)?

Yep! All the support is in the Makefile.

Rendered static HTML pages can be generated via make docs, which runs dex script --outfmt html and outputs pages to a doc/ directory:

$ make docs
stack exec ... dex -- script examples/mandelbrot.dx --outfmt html > doc/examples/mandelbrot.html
stack exec ... dex -- script examples/pi.dx --outfmt html > doc/examples/pi.html

@srush
Copy link
Contributor Author

srush commented Jan 20, 2021

Nice. @dan-zheng maybe it should just push to gh-pages directly? I just figured out you could do that that the other day: https://github.com/harvardnlp/pytorch-struct/blob/master/.github/workflows/docs.yml

@dan-zheng dan-zheng mentioned this pull request Jan 20, 2021
1 task
@dan-zheng
Copy link
Collaborator

@dan-zheng maybe it should just push to gh-pages directly?

Thanks @srush! I filed #477 to track automated GitHub Pages updates, it should be a straightforward improvement!

There's one consideration on the issue that I'd like to discuss with folks before moving forward with automation.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@srush srush changed the title Hotfixes : Tutorial update to latest Tutorial bugfixes and CI integration Jan 21, 2021
@@ -292,9 +292,9 @@ def tableAdd' (x : m => n => Float32) (y : m => n => Float32) : m => n => Float3

' To run this section, move the following binary files to examples:

' `wget https://github.com/srush/learns-dex/raw/main/mnist.bin`
' `wget http://yann.lecun.com/exdb/mnist/t10k-images-idx3-ubyte.gz; gunzip t10k-images-idx3-ubyte.gz`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't unzip a bit more standard for this? I think it's equivalent to gunzip on most Linuxes, but also works on BSD-based systems (macOS).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, is that true? unzip doesn't seem to work for me linux for .gz files. Also gunzip seems to install fine in the macOS CI test. But I don't have a mac to test on. Does unzip work there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I confirm the following works on macOS:

$ wget http://yann.lecun.com/exdb/mnist/t10k-images-idx3-ubyte.gz; gunzip t10k-images-idx3-ubyte.gz
# Produces unzipped t10k-images-idx3-ubyte, 7.5M.

Hope this helps!

Comment on lines +462 to +465
interface Add a
add : a -> a -> a
sub : a -> a -> a
zero : a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm this is a good question... I honestly don't know whether we can do that or not. The worst part is that the MNIST download page doesn't even mention a license, so I'm a bit reluctant to do so 😕 Could we use a different dataset by any chance?

examples/tutorial.dx Outdated Show resolved Hide resolved
examples/tutorial.dx Show resolved Hide resolved
@srush
Copy link
Contributor Author

srush commented Jan 25, 2021

Hmmm this is a good question... I honestly don't know whether we can do that or not. The worst part is that the MNIST download page doesn't even mention a license, so I'm a bit reluctant to do so confused Could we use a different dataset by any chance?

Keras claims: License: Yann LeCun and Corinna Cortes hold the copyright of MNIST dataset, which is a derivative work from original NIST datasets. MNIST dataset is made available under the terms of the Creative Commons Attribution-Share Alike 3.0 license.

Now we are downloading though, so is there an issue? For whatever reason this dataset is always the dataset these are written on.

(For some reason I can't reply to this inline. )

@srush
Copy link
Contributor Author

srush commented Jan 27, 2021

Ping on this one. Think it is ready to go unless I'm missing something.

@apaszke
Copy link
Collaborator

apaszke commented Feb 1, 2021

Ugh, sorry for letting this slip. Yeah the only reason for the delay were the licensing issues. The Keras note might be good enough, but what if we switched to Fashion MNIST? This one is released under MIT, so there would definitely be no issue with downloading it in CI.

@srush
Copy link
Contributor Author

srush commented Feb 1, 2021

Updated. Haha Fashion MNist is fun. It requires literally no changes.

@apaszke
Copy link
Collaborator

apaszke commented Feb 1, 2021

Yeah, that's why I suggested it! LGTM!

@apaszke apaszke merged commit 540f1a1 into google-research:main Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants