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

Macros in wppl programs #153

Closed
ngoodman opened this issue Jul 28, 2015 · 16 comments
Closed

Macros in wppl programs #153

ngoodman opened this issue Jul 28, 2015 · 16 comments

Comments

@ngoodman
Copy link
Contributor

It would be useful to have macros in webppl. It should be able to do this without interfering much with the system by running webppl files through sweet.js before everything else happens. (Perhaps only when a macro is detected?)

If we have macros like this, are there other things that can be simplified by relying on them? E.g. AD, desugarring assignment, ..

@null-a
Copy link
Member

null-a commented Sep 24, 2015

I've been trying to figure out how best to get sweet.js + webppl working in the browser. (As you probably know, a straight-forward application of our current browserify approach doesn't work.) So far I have two possible approaches, neither of which are ideal.

Load sweet.js with RequireJS

This is how the library is loaded into the online sweet.js editor. I got this working with webppl by loading sweet.js with RequireJS and modifying webppl so that sweet.js is passed into run and compile. Downsides with this approach at present include:

  1. We have two different mechanisms for getting code into the browser.
  2. It ends up loading escodegen twice I think.
  3. We have to thread the sweet module through the compilation code as it comes from different places depending on whether we're in node or the browser.
  4. A single webppl.js file is replaced by several files.

If you want to try this:

  1. Checkout the branch.
  2. cd compiled
  3. make clean && make
  4. cd ..
  5. python -m SimpleHTTPServer
  6. Visit http://localhost:8000/compiled/index.html
  7. Call run(<webppl_code>) in the debug console to test.

Patch sweet.js so it can be browserified

It turns out that there are two problems in applying browserify to sweet.js. One comes from the use of fs and require in ways which can't be transformed by the current tools, and the other comes from a bug in brfs. As a proof of concept I worked around this by patching sweet.js and by using an old version of brfs. It's not ideal, but it works. Problems include:

  1. Having to patch sweet.js is brittle.
  2. We don't want to be stuck on an old version of brfs. (Though I suspect we could fix the bug.)

If you want to try this:

  1. Check out the branch.
  2. npm install (Should install [email protected])
  3. cd compiled
  4. make clean && make
  5. Open compiled/index.html in a browser. (open index.html)
  6. Call run(<webppl_code>) in the debug console to test.

Since this will patch sweet.js, you should probably npm uninstall sweet.js and npm install sweet.js once you are through experimenting.

Both of these branches include a couple of macros in header.wppl for testing.

@stuhlmueller
Copy link
Member

The second solution looks promising!

How about we proceed as follows:

  1. Fork brfs.
  2. Fix the bug in our fork (assuming it's not more than a few hours of work).
  3. Switch the package.json brfs dependence to our fork (like this).
  4. Create a PR with the bugfix.
  5. If/when it's accepted and makes it into npm, switch package.json back to the official brfs source.

Do the same thing to make sweet.js compatible with browserify.

The drawback of this approach is that we have to maintain brfs and sweet.js forks for a while, and potentially indefinitely if the original maintainers don't accept our pull requests. However, since our changes are pretty minimal, I expect that it's little work to merge in upstream changes every now and then, and hopefully we can create pull requests that are unconditional improvements and have a good chance of being accepted.

I'm also open to other approaches—this is just what seems good to me right now based on the ideas in your comment; there may well be other paths that I'm missing.

@null-a
Copy link
Member

null-a commented Sep 28, 2015

The second solution looks promising!

I agree that of the two this would be the better way to go.

I've spent some time tracking down exactly where things go wrong with brfs. One possible snag I came across is that the problem we're hitting may (at it's core) be a design decision rather than a bug, which might make changing things upstream trickier. I've opened a ticket in the hope of finding out whether or not we're likely to make any progress with this. The changes that need to be made to sweet.js are somewhat dependant on what happens with brfs, so I think it makes sense to hold off doing anything with that for now.

@null-a
Copy link
Member

null-a commented Sep 29, 2015

I have a new branch which is a little easier to set-up than those I linked to earlier. To use it with node just git clone and npm install. cd compiled && make no longer patches sweet.js, instead it's pulled from a fork on GitHub.

@null-a
Copy link
Member

null-a commented Sep 29, 2015

The way that I'm currently integrating sweet.js into the compilation pipeline looks something like this:

concat(header.wppl, programCode)
=> sweet transform
=> webppl transforms

This works, but the concatenation of the header and program makes the following tricky:

  1. Caching the result of compiling the header. (Cache compiled webppl header #62)
  2. Applying the caching transform to only the program, as we do at present. (Caching transform is applied to wppl files in packages but not header.wppl #202)

The approach I'd like to take is to do something like:

map(sweet transform, [header.wppl, programCode])
=> map(conditional caching transform, _)
=> concat
=> webppl transforms

But that doesn't work because sweet.js always renames all of the identifiers in the header (e.g flip might become flip$606) but of course it can't make the corresponding changes to the program.

This raises the question: Is it necessary to expand the macros in a single global pass, or is it possible to do it per-file while retaining hygiene. (I've not been able to convince myself either way yet.)

I noticed that @iffsid uses the sweet.js readableNames option in the AD branch. This certainly helps in some cases, but will it work in general?

@dritchie
Copy link
Contributor

I've also been using readableNames in my own stuff and have been able to do per-file macro transforms without issue.

One other thing: at least in the case of AD, it might also be a good idea to have the macro transform be a conditional transform (as in, only apply it if an inference algorithm that needs it is used). The AD operator overloading adds dynamic type checks that, from the experiments I did a little while ago, slow things down by at least factor of ~2 for numerics-heavy code.

@stuhlmueller
Copy link
Member

I have a new branch which is a little easier to set-up than those I linked to earlier. To use it with node just git clone and npm install.

Nice, worked for me in the browser. For example (to make it easier for others who might want to try this):

run('operator (<>) 0 left { $val, $f } => #{ $f($val) };' + 
    'var double = function(x){return x*2};' + 
    '1 <> double <> double')

webppl.js:57917 compile: 1196.243ms
index.html:9 4
webppl.js:57929 run: 1.240ms

This raises the question: Is it necessary to expand the macros in a single global pass, or is it possible to do it per-file while retaining hygiene. (I've not been able to convince myself either way yet.)

One thing we'd like to do is define macros in the header and in packages, and have them applied to the (user-specified) program code. So, any per-file expansion would have to allow for that.

@null-a
Copy link
Member

null-a commented Sep 30, 2015

I've also been using readableNames in my own stuff and have been able to do per-file macro transforms without issue.

Great, thanks! I've switched my branch to use this too. One possible problem is that it doesn't work with ES6 syntax at present, though that's unlikely to be permanent. I doubt this is a problem for us, but I thought I should mention it.

One thing we'd like to do is define macros in the header and in packages, and have them applied to the (user-specified) program code

For macros in the header, the way I'm doing this right now is by having the contents of a new file (macros.sjs) be applied to both header.wppl and to the program. This works fine.

Since we concatenate all wppl files (user specified program + wppl files from packages) before compilation, then (for better or worse) all wppl files will get all macros at present.

Even if that behavior is desired, this approach breaks down if we choose to make it possible for individual wppl files to opt-out of particular transforms. (Because in that case we'd no longer be concatenating all wppl files and transforming them together. See #202.) The most straight-forward solution would be to have a new macros field in package.json allowing packages to specify additional *.sjs files. We could easily apply macros defined in such files to just the user-specified program. This also has the side-effect of making macros defined within wppl files be file local, which seems like it might be useful.

@stuhlmueller
Copy link
Member

For macros in the header, the way I'm doing this right now is by having the contents of a new file (macros.sjs) be applied to both header.wppl and to the program. This works fine.

Sounds good.

Since we concatenate all wppl files (user specified program + wppl files from packages) before compilation, then (for better or worse) all wppl files will get all macros at present.

My first reaction was that macros in user-specified programs shouldn't transform the header, but actually it's not obvious. Let's go ahead with the solution you propose, and then if it turns out that we want to do things with macros that it doesn't support, it hopefully won't be too difficult to adjust.

The most straight-forward solution would be to have a new macros field in package.json allowing packages to specify additional *.sjs files. We could easily apply macros defined in such files to just the user-specified program.

Including the wppl files in the package, presumably?

@null-a
Copy link
Member

null-a commented Sep 30, 2015

Including the wppl files in the package, presumably?

That seems reasonable. What about wppl files in other packages?

@stuhlmueller
Copy link
Member

What about wppl files in other packages?

I think a good solution would rely on explicit dependencies between packages for this (#139, #209, #211), and would only apply macros that are provided by other packages that a package depends on. So, if the top-level program requires two packages in parallel, neither of these would affect the other with respect to macro expansion.

If you agree that that's a reasonable direction going forward, then maybe the thing to do now is to make the macro system behave such that it is consistent with what will happen when we introduce dependencies between packages. That is, core webppl macros apply to everything, including packages, and macros in a package apply to wppl code in the same package and to user-specified code, but not to other packages.

@null-a
Copy link
Member

null-a commented Oct 1, 2015

So, if the top-level program requires two packages in parallel, neither of these would affect the other with respect to macro expansion.

It sounds reasonable, although I guess we may want them to interact for the same reason we might want user macros to transform the header.

core webppl macros apply to everything, including packages, and macros in a package apply to wppl code in the same package and to user-specified code, but not to other packages

I've updated the branch to do this and it's working in node and the browser. (It's still work-in-progress though, the --compile option is broken for example.)

@null-a
Copy link
Member

null-a commented Oct 2, 2015

For reference, here's my understanding of the situation with sweet.js and ES6.

As mentioned earlier, the readableNames option we've been using to allow per-file macro expansion doesn't support ES6 syntax. This is mentioned in the sweet.js docs.

The reason for this is that sweet.js depends on escope to do the heavy lifting behind readableNames and they currently use an old version which itself doesn't support ES6. I'm not sure how much effort it would take to upgrade, whether anyone's actively working on it, etc.

I should note that while the use of readableNames is convenient, it's not unimaginable that we could get things working without it.

@stuhlmueller
Copy link
Member

If having readableNames makes things easier, we can hold off on ES6 for a while. Eventually we'll need to switch, and if there were no drawbacks I'd do it earlier rather than later, but if there is in fact an issue which may get resolved by waiting, then we can wait.

@wuxianliang
Copy link

sweet.js is refactoring on Babel. sweet-js/sweet-core#485

@ngoodman
Copy link
Contributor Author

ngoodman commented Nov 1, 2015

thanks for the pointer! i guess we'll have to wait and see if this impacts us.

@null-a null-a mentioned this issue Nov 17, 2015
@null-a null-a closed this as completed Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants