`markovChain.findStationaryDistribution` is currently written such that
it blows the function call stack size if it is called with >200k nodes.
This commit effects a small refactor so that the delta is computed in a
loop over elements in the distribution, rather than creating a massive
function arguments list. This results in the correct behavior.
Test plan:
We discussed offline and decided not to test it. Reasons:
- We could factor out `computeDelta` and test that function in
isolation, but I think this provides very, very little value. The class
of regression it protects against is almost precisely the case where
someone reverts this commit.
- We could test the entire `findStationaryDistribution` function, which
is more valuable, but requires a large, slow test case (~1 sec)
- This is a bug that is unlikely to re-occur or to slip by unnoticed if
it does. It's also unlikely to surface in future refactoring.
See #286 for context.
I also upgraded client code in src/app.
Test plan:
Unit tests are extensive, including testing that `get`, `ref`, and
constructors are overriden on every `GitReference` and `GitPorcelain`
type that is exposed to clients.
Paired with @wchargin
See #286 for context. There are a few miscellaneous changes in
src/app/credExplorer to change clients to use the new API.
Test plan:
New unit test were added, and existing behavior is preserved. Most of
the functionality of the GitHub porcelain was already well tested.
Paired with @wchargin
`NodeReference` and `NodePorcelain` act as abstractions over the two
states a Node can be in.
- We might have the address of a node (because some edge pointed to it),
but don't actually have the Node in the graph. In that case, we can do
some queries on the node (e.g. find its neighbors), but can't access
the payload. This corresponds to having a `NodeReference`.
- We might have the node in the graph. In that case, we can access a
`NodePorcelain`.
The main benefit this abstraction brings is type-safety over accessing
data from a `NodePayload`. Previously, the coding conventions encouraged
clients to ignore the distinction, and the type signatures incorrectly
reported that many payload-level properties were non-nullable. Now, the
`get` method that mapp a `Reference` to a `Payload` is explicilty
nullable.
Given a `NodePorcelain`, it's always possible to retrieve the reference
via `ref()`. Given the `NodeReference`, you might be able to retrieve
the `NodePorcelain` via `get()`.
Clients that subtype `NodePorcelain` and `NodeReference` should, in
general, override the `ref()` and `get()` methods to return their
subtype. We also recommend having subclasses overwrite the constructors
to take a base `NodePorcelain` and `NodeReference` respectively
(although the base classes take a `Graph` and `address` as constructor
arguments).
Test Plan: Inspect the unit tests, they are pretty thorough.
Paired with @wchargin
Summary:
A few changes were made to code that is correct (as far as I can tell),
but for which Flow can no longer infer a type parameter. The change is a
bit more annoying than it otherwise would be, because this particular
file is run directly via node and so must use Flow’s comment syntax for
type annotations, but Prettier breaks such comments in the cases that we
need. We work around this by rewriting the original code to avoid the
need for comments.
Test Plan:
In addition to standard CI, run `yarn build` and then run a server from
`build/`, to see that the production build produces a working bundle.
(That the app loads and renders is sufficient.)
wchargin-branch: upgrade-flow-v0.72.0
Summary:
Instead of having one function that returns a union, we present two
functions, each of which returns a more specific type.
Paired with @decentralion.
Test Plan:
Existing unit tests pass, with sufficient modifications.
wchargin-branch: separate-issueByNumber-prByNumber
This commit adds "Node Porcelain" for the Git plugin. Node porcelain is
a wrapper over a graph and node address, which makes it possible to
access payload data and adjacent nodes via a familiar object-oriented
API.
I believe this porcelain provides substantially better legibility and
usability for the Git plugin. Consider that it is now easy to see what
relationships each Git node type can have by reading the method
signatures in the porcelain, rather than needing to inspect all of the
Edge types in types.js.
This porcelain has slightly different conventions from the porcelain in
the GitHub plugin, although the APIs are very similar. I intend to
follow this commit with two more: one that switches clients of the Git
plugin to use the porcelain, and another that refactors the GitHub and
Git porcelains to use a base Porcelain implementation in src/core.
Test plan:
Examine the public API of the Git porcelain (this is unlikely to change
much), and its corresponding test code.
`yarn travis --full` passes.
Summary:
There are substantive options for `convergenceThreshold` and
`maxIterations`, as well as the output option `verbose`. This change is
made in preparation for extracting this function into `markovChain`,
where we will add unit tests for it.
Test Plan:
Behavior of `yarn start` is unchanged.
wchargin-branch: configurable-findstationarydistribution
Summary:
This commit slightly reorganizes the internals of `basicPagerank` to use
the `SparseMarkovChain` type from the `markovChain` module.
Test Plan:
Behavior of `yarn start` is unchanged.
wchargin-branch: use-sparsemarkovchain
Summary:
This function is mostly useful for easily describing Markov chains in
test cases.
Test Plan:
Unit tests added. Run `yarn test`.
wchargin-branch: sparseMarkovChainFromTransitionMatrix
Summary:
For now, this module has just two types: `Distribution` and
`SparseMarkovChain`. We’ll gradually pull code from `basicPagerank` into
this module, adding unit tests along the way.
Test Plan:
None required.
wchargin-branch: markov-chain-module
Summary:
See #66 for more context. This yields the following performance
improvements for me, on the SourceCred graph with 11 072 nodes and
20 250 edges:
- Loading a graph from disk is improved overall from 1172 ms to 292 ms
(4.0× improvement).
- The full pipeline for basic PageRank, from button press to final
render, is improved from 8.44 s to 4.39 s (1.9× improvement).
- The PageRank preprocessing step, which involves turning the graph
into a typed array Markov chain, is improved from 2430 ms to 573 ms
(4.2× improvement).
- The PageRank postprocessing step, which involves turning the typed
array distribution into an `AddressMap` distribution, is improved
from 83.53 ms to 4.81 ms (17× improvement).
- The `PagerankTable` React component `render` method (constructing
the virtual tree only, not diffing or embedding into the DOM) is
improved from 1708 ms to 332 ms (5× improvement).
The core matrix computations of PageRank are unaffected, because they do
not use the `AddressMap` abstraction.
Test Plan:
Existing unit tests suffice.
wchargin-branch: fast-addressmap
Summary:
This takes `AddressMap` access, and therefore JSON stringification, off
the critical path, resulting in a significant performance increase. The
resulting code is much faster than the original TFJS implementation. On
my laptop, we can run about 300 iterations of PageRank per second on a
graph with 10 000 nodes and 18 000 edges (namely, the SourceCred graph).
Paired with @decentralion.
Test Plan:
Run `yarn start` and note that the cred attribution for SourceCred is
roughly the same as before… but is created faster.
wchargin-branch: pagerank-typed-arrays
Summary:
We’re not convinced that using TFJS at this time is worth it, for two
reasons. First, our matrix computations can be expressed using sparse
matrices, which will improve the performance by orders of magnitude.
Sparse matrices do not appear to be supported by TFJS (the layers API
makes some use of them, but it is not clear that they have much support
their, either). Second, having to deal with GPU memory and WebGL has
already been problematic. When WebGL PageRank is running, the machine is
mostly unusable, and other applications’ video output is negatively
affected (!).
This commit rewrites the internals of `basicPagerank.js` while retaining
its end-to-end public interface. We also add a test file with a trivial
test. The resulting code is not faster yet—in fact, it’s a fair amount
slower. But this is because our use of `AddressMap`s puts JSON
stringification on the critical path, which is obviously a bad idea. In
a subsequent commit, we will rewrite the internals again to use typed
arrays.
Paired with @decentralion.
Test Plan:
The new unit test is not sufficient. Instead, run `yarn start` and
re-run PageRank on SourceCred; note that the results are roughly
unchanged.
wchargin-branch: pagerank-without-tfjs
We were sorting low-to-high, and then reversing. We can just sort
high-to-low.
Test plan: No behavior change (confirm by interacting with the Cred
Explorer).
Paired with @wchargin
Test plan: Open the cred explorer, and use the + sign to expand the
neighbors. Observe that those neighbors are now sorted by cred.
Paired with @wchargin
Test plan: Open the cred explorer, and try clicking the + signs. They
will expand a recursive table showing the neighbor nodes and their cred.
Paired with @wchargin
`PagerankTable` is forked from `ContributionList`.
Test plan: I took it for a spin and it seemed OK. I'm not inclined to
write formal tests because we are in rapid prototyping mode stil.
Test Plan:
Load the cred explorer for the first time to see two empty boxes.
Refresh to see the same thing. Then, add some content to each box.
Refresh and see the same content.
wchargin-branch: cred-explorer-localstorage
This re-organizes the GitHub porcelain tests to be:
- organized by each method signature, rather than having blocks that
test many different methods on each wrapper
- make extensive use of snapshot tests for convenience
Test plan: Inspect the new unit tests, and the corresponding snapshots.
It should be relatively easy to do this because you can copy+paste the
urls to verify the properties.
Summary:
This fixes two problems in the previous version:
- A new JS file not checked into git, but with a `@flow` directive,
would cause `ensure-flow` to fail, because one list of files was
from `git grep` and the other was from `find`.
- Only the hard-coded directories `src config scripts` were searched.
Now, we search all JS files checked into Git, except for some hard-coded
exceptions, namely `flow-typed`.
Test Plan:
1. Add `foo.js`, not checked into Git. Note that `ensure-flow` passes.
2. Add `@flow` to `foo.js`, and note that `ensure-flow` still passes.
3. Remove `@flow` from `.eslintrc.js`, and note that `ensure-flow`
fails and nicely prints the filename. (Note: this file is at the
repository root.)
4. Create a file `echo stuff >$'naughty\nfilename.js'`, and note that
`ensure-flow` has the correct behavior in both positive and
negative cases.
wchargin-branch: ensure-flow-improvements
Summary:
Even though it’s not really a source file, and it lives at the
repository root, it might as well have typing to make sure that we don’t
do anything really dumb.
Test Plan:
`yarn flow` still passes.
wchargin-branch: flow-eslintrc
Test plan: Unit tests were added.
(Note: I haven't tested the error case, when there are an invalid number
of parents. I think this is OK for now, but I'm disclosing this so
reviewers can easily take issue with it. I'm planning to re-organize the
test code to be by method rather than by wrapper type (so the wrappers
section doesn't keep being a kitchen sink) and will hopefully
put that test in.)
Also updates the GitHub porcelain.
Existing observable behavior is unchanged, except that performance may
be improved for issueOrPrByNumber.
A bug that would afflict a multi-repository graph (namely, that calling
`repo.issues()` would get all issues for all repositories) is
pre-emptively removed. No test cases were added as we do not yet support
multi-repository graphs.
Test plan: existing unit test coverage is sufficient.
Currently, the `authors` method is attached at the Repository level.
This is incorrect; it actually finds all the authors in the graph, not
all the authors of that repository. This commit moves the method to the
correct class.
Test plan: This function is only used in test code. The tests still
pass.
Summary:
We don’t expect the results to be of good quality right now. Rather,
this gives us a starting point from which to iterate the algorithm.
The convergence criterion also needs to be adjusted. (In particular, it
should almost certainly not be a constant.)
Test Plan:
Run `yarn start`. Select a graph, like `sourcecred/example-github`. Open
the JS console and click “Run basic PageRank”. Watch the console.
wchargin-branch: basic-pagerank
Summary:
This commit enables the cred explorer to fetch pre-generated graphs. The
form has poor UX, but gets the job done. (To do later: saving in
localStorage, allowing fetching the list of possible graphs, linking the
submit button so that it’s triggered by `Enter`, etc.).
Test Plan:
Set up with `node ./bin/sourcecred.js graph sourcecred sourcecred`, then
run `yarn start` and check the following cases:
- success case: `sourcecred`/`sourcecred`
- error case: `sarcecrod`/`sarcecrod` (nice console error)
- error case: the repository owner or name is an empty string or has
invalid characters, like `../../secret.txt` (nice console error)
wchargin-branch: fetch-graphs
Summary:
This commit changes `yarn start` to run a production version of the API
server, which serves static files from a pre-built directory and also
handles API requests.
Test Plan:
```shell
$ yarn build
$ yarn backend
$ node ./bin/sourcecred.js start -d /tmp/srccrd &
$ mkdir -p /tmp/srccrd
$ echo hello >/tmp/srccrd/world
$ curl -s localhost:4000/ | file -
/dev/stdin: HTML document, ASCII text, with very long lines, with no line terminators
$ curl -s localhost:4000/api/v1/data/world
hello
```
wchargin-branch: cli-start-prod-server
Test plan: Run `yarn start`, and observe that the Cred Explorer is now
included in the nav bar, and can be selected, in which case a short
message is displayed.
testUtil contains some useful configuration endpoints for our frontend
testing. This commit moves it out of the artifact editor and up to
`src/app`.
Test plan: `yarn travis` passes.
@wchargin suggested that the entity-wrapping logic in porcelain
reference handling should be factored out as its own method. This was a
great suggestion; it will be very useful for plugin consumers, and it
also results in better test coverage.
Test plan: The new unit tests are nice. For your own safety, do not
question or quibble with the magnificent foo plugin.
Summary:
This way, our frontend can talk to a backend that can read from the
filesystem (among other things).
Paired with @decentralion.
Test Plan:
```
$ yarn backend
$ SOURCECRED_DIRECTORY=/tmp/srccrd yarn start
$ # verify that the browser looks good
$ mkdir /tmp/srccrd
$ echo hello >/tmp/srccrd/world
$ curl localhost:3000/api/v1/data/world
hello
$ curl localhost:4000/api/v1/data/world
hello
```
wchargin-branch: webpack-proxy
It was doing some clever array construction that added possible booleans
to the array, then filtered them out. To make the typing simpler for
Flow's inspection, we now only put string elements in the array.
Test plan: `yarn travis --full` passed, and the CLI still works.
Summary:
- The value of `process.stdout.isTTY` is either `true` or `undefined`.
Flow (reasonably) dislikes this, so we add an explicit check.
- More `package.json` burnination.
Test Plan:
Note that `require("./package.json").proxy === undefined` in the Node
console, and that `yarn start` works.
wchargin-branch: flow--scripts-start
- scripts/backend.js: We incorrectly set an environment variable to
a boolean, when in fact it must be a string. Fixed it to set a string
value "true", and updated usage in config/babel.js
- scripts/test.js: No changes
- scripts/build.js: Removed a call to printHostingInstructions, so that
we don't need to require the package.json.
Test plan:
`yarn travis --full` passes, and the SourceCred cli still works.
Fixing the flow error corresponded to (correctly) documenting that the
GitHub token is mandatory, not optional.
Test plan: `yarn travis --full` still passes.
Dandelion Mané
William Chargin