mhtess / rwebppl

RWebPPL, an R interface to Webppl http://webppl.org
22 stars 7 forks source link

yoke defaults to webppl versions #53

Closed hawkrobe closed 7 years ago

hawkrobe commented 7 years ago

Addresses #50, #42, #30

mhtess commented 7 years ago

When I use install_webppl() to change versions, I get the following warnings:

mv: rename node_modules/webppl/ to ./webppl/: Directory not empty mv: rename node_modules to webppl/node_modules/node_modules: Directory not empty done

mikabr commented 7 years ago

@mhtess I think that's coming from rearrange-webppl.sh, whose function I don't understand

hawkrobe commented 7 years ago

@mikabr : MH and I just chatted about this quickly in person. The rearrangement shell script was necessary to make symlink'd webppls look the same as npm'd webppls. If it was always being installed the same way, we wouldn't need to worry about it. Unfortunately, even after getting rid of symlinks, we still have two ways of installing to support the things we want to support (github & npm). So the quick fix would just be to tweak the rearrangement script to make those two look the same. But the more maintainable thing would probably be to stick to a single modality of installing webppl.

Because the official npm releases correspond to github tags (i.e. 'v0.9.6') we thought the best way would be to install everything through github? It'll be the same logic as the code you've already written, but with git commands (e.g. git tag -l) swapped in for the npm commands... Thoughts?

mikabr commented 7 years ago

How does installing webppl from npm and git make it look different?

mikabr commented 7 years ago

To elaborate, it's not clear to me how they should be any different, in which case we can just remove the rearrange script. If they are somehow different, installing from github by tag seems reasonable. I think cloning the repo is slower than using npm (?) so it's not necessarily better to switch to always installing from github.

mhtess commented 7 years ago

I think by "look the same" I think it's meant that where the various files get installed to looks the same. So the webppl_rearrange would be used to reorganize the file structure. This is a hunch though

hawkrobe commented 7 years ago

Good point about git being slower!

By 'look the same' I mean the directory structure they set up:

So the rearrangement script just flattens out the npm installation to match the git installation by pulling webppl out of node_modules and moving its dependencies inside it. Does that make sense?

The options as I seem them (in order of ease) are:

  1. only run the rearrangement script when installing from npm (which is what we were doing before this change, but it got moved outside the if statement)
  2. only install from git so that we don't need a rearrangement script
  3. do some weird conditional logic to switch the webppl path based on where we installed from
hawkrobe commented 7 years ago

One additional thing separate from the above decision: at the beginning of the install function, we should probably check whether js/webppl already exists and remove it if so. It currently works the first time, but then errors out the second time.

mhtess commented 7 years ago

Is git being slower really an issue? What kind of time difference are we looking at here?

Since this command is not run very often, I am leaning toward robert's option (2) as the best.

hawkrobe commented 7 years ago

Is git being slower really an issue? What kind of time difference are we looking at here?

It's about 1 minute slower with the current build... Enough to be really annoying.

mhtess commented 7 years ago

I see. What are the impediments to (1)?

hawkrobe commented 7 years ago

I see. What are the impediments to (1)?

No impediments! It's just one line of code essentially fixing it back to what it was before mika's overhaul moved it outside an if block (I've tested it and it works; just didn't want to do it if you & mika thought the other options were better).

mhtess commented 7 years ago

@mikabr will you sign off on this?

mikabr commented 7 years ago

👍

mhtess commented 7 years ago

okay @hawkrobe will you make the change? then i'll double-check on my system and merge

hawkrobe commented 7 years ago

All done

mhtess commented 7 years ago

Looks great! Thanks a lot team