The implementation is adapted from our previous implementation, but has
been refactored to be more appropriate for our generator-function
approach.
The unit tests comprehensively explore a simple example graph, testing
the following cases:
- Direction filtering (IN/OUT/ANY/unspecified)
- Node types
- Edge types
- Self-loop edges
- Dangling edges
- Removed edges don't appear
Test plan: Carefully inspect the added unit tests.
Paired with @wchargin
Summary:
Based on code originally paired with @decentralion.
Test Plan:
Unit tests added. Running `yarn travis` is sufficient.
wchargin-branch: v2-edges
Summary:
Previously, our CI script would run `yarn backend`, which has the
side-effect of erasing the `bin/` directory. By itself, this is not
great, but not awful. However, this frequently triggers a race condition
in Prettier, causing the `check-pretty` step of the build to fail. (More
details: https://github.com/prettier/prettier/issues/4468.)
This patch changes the CI script to build the backend scripts into a
temporary directory.
Test Plan:
Before applying this patch: `yarn backend` and then `yarn travis`. If
this consistently causes a Travis failure due to `check-pretty`, then
your machine can reproduce the race condition that we‛re trying to
eliminate. (Otherwise, you can try creating a bunch more Git history…
I’m not really sure what to say. It is a race condition, after all.)
Then, apply this patch, and repeat the above steps; note that the error
no longer occurs, and that the build output is to a temporary directory.
wchargin-branch: ci-preserve-bin
Given that we have reified the type `PluginFilter` as a consistent way
to filter nodes and edges by plugin and type, we should also have
re-usable logic for that end.
This commit adds `addressFilterer`, which takes a `PluginFilter` and
returns a function that checks whether a given address matches the
filter. Right now, only the `nodes` function uses it.
For added safety, `addressFilterer` does runtime validation that the
required `plugin` property was set.
Test plan: See included unit tests.
As of this commit, adding the comment `//$ExpectFlowError` in flow-typed
code asserts that the next line must cause a flow error. If it does, no
error or warning is generated. If it does not, then this produces a flow
warning, which is visible to developers running `yarn flow` and
additionally causes travis to fail.
Test plan:
- As committed, `yarn travis` passes.
- I added `//$ExpectFlowError` above some line of flow-checked code which does
not currently throw an error. Afterwards, `yarn travis` failed (and a
helpful message was displayed in console on running `yarn flow`)
- I added the following bad code into one of our files:
```javascript
//$ExpectFlowError
const foo: string = 3;
```
As expected, `yarn flow` and `yarn travis` both passed.
Summary:
It’s critical, even more so than usual, that the “base reference”
property of a `DelegateNodeReference` be a private property, because
this class is designed for inheritance. In ECMAScript 6, we can achieve
this by giving the property a `Symbol` key instead of a string key.
Unfortunately, Flow doesn’t know about `Symbol`s, so we need a few casts
through `any`, but they are localized to as small a scope as possible.
Test Plan:
Unit tests added. Note that they pass both before and after this change.
wchargin-branch: symbol-base-ref
Summary:
The use of `tee /dev/stderr` failed when running as a child process
under Node for some reason. (I haven’t been able to figure out why—it
works fine when run as a standalone script or when run as a child
process under Python.) This is also technically Linux-specific, so I’ve
changed it to use a process substitution. After looking around for a
bit, there doesn’t seem to be a way to do this in a way that is
portable, uses only POSIX shell features, and doesn’t create temporary
files all at the same time, so the script is now run under `bash`.
Test Plan:
Run `yarn travis` and note that the `ensure-flow.sh` output no longer
contains the line `tee: /dev/stderr: No such device or address`.
wchargin-branch: no-tee-devstderr
This commit adds the following methods to the `Graph`:
* `addNode`
* `removeNode`
* `ref`
* `node`
* `nodes`
* `equals`
The graph now supports adding nodes, with thorough testing. Other
methods were implemented as necessary to test that `addNode` was
implemented properly.
Also, we've made a slight change to spec: `nodes` (and other filter
options) accept a `PluginFilter` object, which, if present, must
specify a plugin and may specify a type.
I've taken the opportunity to re-write the graph test code. Instead of
having a complicated `graphDemoData` file that creates a graph with many
different nodes, I've created an `examplePlugin` which makes it trivial
to instantiate new simple Foo and Bar nodes on the fly. Then, test cases
can construct a small graph that is clearly appropriate for whatever
functionality they are testing.
Test plan: Unit tests were added, travis passes.
Test Plan:
Run `yarn flow stop; yarn travis | cat` and note the absence of the
really long line that has ~2500 bytes of “Server is initializing”.
wchargin-branch: quiet-flow-server
Currently, when filtering by type (e.g. in neighbors), we require only a
type string. This is a design flaw, as if two plugins both define an
"ISSUE" type, either plugin may unexpectedly receive the other plugin's
nodes or edges.
We fix the flaw by explicitly binding the plugin name and type field
together as `PluginType`.
Test plan: Flow passes, and so does the following invocation to check
our design doc:
```
cat <(printf '// @flow\n') \
<(awk -v RS='```[a-z]*' '(NR+1)%2' \
src/core2/address_payload_unification_design.md) \
| yarn -s flow check-contents
```
- Add the `plugins` method to graph, and have the constructor take them
- Add in demo `PluginHandlers` to the `graphDemoData`
- Add unit tests for `plugins` methods
Test plan: Travis
Paired with: @wchargin
Summary:
In fact, a constructor that only calls `super` is useful: it specifies
the API for constructing an object of a given class without needing to
traverse its prototype chain. (That constructors are inherited at all is
arguably a design flaw.)
Test Plan:
None.
wchargin-branch: yes-useful-constructor
Also, rename `NodeDelegateReference` to `DelegateNodeReference` in the
design doc.
Test plan:
Didn't add tests, as the implementation is trivial. Flow passes.
src/core2/graph.js has the types and Graph class scaffold as
described in address_payload_unification_design.md.
Also, we decided to modify the design doc to include a type parameter
for Edges (although we aer open to removing if it becomes cumbersome).
Test plan:
`yarn flow` passes, and the design doc still typechecks too.
Paired with @wchargin
Since this document was originally written, we've iterated on the design
a bit. @wchargin and I have re-reviewed the doc, and we think it's now
up-to-date with our plans.
Test plan: I ran the following command to bring the code into my
clipboard:
```
❯ awk -v RS='```[a-z]*' '(NR+1)%2' src/core2/address_payload_unification_design.md | xsel -ib
```
Test plan:
Pasting this into https://flow.org/try produced no errors.
Paired with @wchargin
In #190, @wchargin lays out an ambitious proposal for refactoring away
the graph's address-payload distinction. This has proven to be a
complicated refactor to land, so we are going to achieve it by forking
parts of the project into v2, updating incrementally in v2, and then
replacing original components with their v2 versions.
Test plan:
No new code added. `yarn travis` passes.
Paired with @wchargin
Given that we are undergoing a major world-changing refactor (#190), all
outstanding code needs to be refactored to use the new conventions. We
don't actually use the Artifact Plugin yet, and reading the code, it
needs non-trivial rewrites to be in sync with the new world.
Rather than maintain it now, I am deleting it; we can regain the context
when the time is ripe to setup and integrate the plugin.
Test plan: Travis passes. `yarn start` produces no references to the
artifact editor.
Summary:
Placing `STOPSHIP` or `stopship` (or any case variant) in any file
tracked by Git will now cause a `yarn travis` failure. If you need to
use this string, you can concatenate it as `"stop" + "ship"` or
equivalent.
Test Plan:
In `travis.js`, change `"check-stop" + "ships"` to `"check-stopships"`,
and note that this causes the build to fail with a nice message. Note
that this also causes `check-stopships.sh` to fail even when invoked
from an unrelated directory, like `src`.
wchargin-branch: check-stopships
Summary:
This just slows down commits by a few seconds. We `check-pretty` in
Travis, so this doesn’t actually catch anything—and, anecdotally, it has
never caught anything for me because I automatically run `prettier` on
save and also (almost) always run Travis before pushing.
Test Plan:
Run `git commit --amend --no-edit` and note that it is now fast!
wchargin-branch: no-lint-on-commit
Summary:
Improves the result of the preceding commit to strip out cruft. Paired
with @decentralion.
Test Plan:
```shell
$ ! git grep -Fi -e simple -e advanced src/core/graph*
```
Also review `awk '/(describe|it)\(/' src/core/graph.test.js`.
wchargin-branch: simplify-simple-advanced
Summary:
These tests have apparently been borked since the beginning. They were
testing properties about nodes instead of edges, had the wrong test
names, and had the LHS/RHS backward… this should make them more useful.
Test Plan:
Unit tests suffice, I suppose.
wchargin-branch: tests-for-missing-edges
Summary:
We now use the “advanced” graph everywhere. Happily, all tests still
pass!
Starting from `:%s/\%(advanced\|simple\)MealGraph/mealGraph/gc`, only a
few tests had to be manually changed: in particular, the `#equals` tests
for “returns false when the ?HS has nodes missing in the ?HS” became
trivially incorrect by the above substitution, and were therefore
changed to use more appropriate inputs.
Paired with @decentralion.
Test Plan:
Existing unit tests suffice.
wchargin-branch: remove-simple-graph
Summary:
We had three graph merging functions: `merge`, `mergeConservative`, and
`mergeManyConservative`. Of these, `merge` was never used outside of
code directly testing its behavior, and `mergeConservative` is a
strictly inferior version of `mergeManyConservative`. This commit
removes `merge` and `mergeConservative`, and renames
`mergeManyConservative` to `mergeConservative`.
Paired with @decentralion.
Test Plan:
Existing unit tests suffice; some useless tests pruned.
wchargin-branch: mmeerrggee
As previously implemented, the Graph was polymorphic in its NodePayload
and EdgePayload. This was a lot of bookkeeping for very
little apparent benefit. In general, graphs may be constructed with
foreign plugins, so it's hard to do anything with this information.
In my experience, having the Graph polymorphism has never caught a bug,
and has led to lots of boilerplate code, especially typing `Graph<any,
any>`. I view the fact that in #286 we added a new core `NodeReference`
concept, which always types its Graph as `<any, any>`, as strongly
suggestive that this was not going to provide any lasting value.
In this commit, I've removed the Graph polymorphism. Note how in many
cases where we were typing the graph, it provided no value, as evidenced
by the fact that the imported Node and Edge types were used no-where
else in the file other than in the Graph declaration.
Test plan:
Removing extra typing information is very unlikely to cause regressions.
`yarn flow` and `yarn lint` both pass.
Summary:
This is an implementation-only, API-preserving change to the `Graph`
class. Edges’ `src` and `dst` attributes are now internally represented
as integer indices into a fixed ordering of nodes, which may depend on
non-logical properties such as insertion order. The graph’s serialized
form also now stores edges with integer `src`/`dst` keys, but the node
ordering is canonicalized so that two graphs are logically equal if and
only if their serialized forms are equal. This change substantially
reduces the rest storage space for graphs: the `sourcecred/sourcecred`
graph drops from 39MB to 30MB.
Currently, the graph will have to translate between integer indices and
full addresses at each client operation. This is not actually a big
performance regression, because it is just one more integer-index
dereference over the previous behavior, but it does indicate that the
optimization is not living up to its full potential. In subsequent
changes, the `NodeReference` class will be outfitted with facilities to
take advantage of the internal indexing; a long-term goal is that
roughly all operations should be able to be performed within the indexed
layer, and that translating between integers and addresses should only
happen at non-hot-path API boundaries.
This diff is considerably smaller and easier to read with `-w`.
Paired with @decentralion.
Test Plan:
I inspected the snapshots for general form, and manually verified that
the indices for one edge were correct (the MERGED_AS edge for the head
commit of the example repo). Other than that, existing unit tests mostly
suffice; one new unit test added.
wchargin-branch: graph-indexed-edges
This commit adds explicit versioning to the `Graph` and `AddressMap`
JSON representations, using the new `compat` module. This will make it
safer to change the serialization format for these classes.
(Note: I don't expect we'll add backcompat handlers for these classes
soon, but having versioning means that we can change the serialization
format in a way that breaks old data cleanly and explicitly, rather than
introducing undefined behavior.)
Test plan: The changes are slight, and well-captured by the snapshot
tests. Note that after this commit, the SourceCred commands will fail on
old data, so old data will need to be regenerated.
This commit adds the `src/util/compat` module, which is responsible for
data compatibility. It provides two methods: `toCompat` and
`fromCompat`.
`toCompat` allows an object to be versioned with a type and version
string. Doing so wraps the object in an array whose first element has
`version` and `type` fields.
`fromCompat` allows loading compatibilized data. It takes a `type` and
`version` expectation, as well as optional processors for various data
versions. It throws an error if the data was not compatibilized, or if
it is the wrong type, or if it has an unsupported version.
The versioning is designed to be composable; see the final test block
for an example of loading a structure with nested versioning.
Test plan: Carefully inspect the included unit tests, and feel free to
propose more.
For context, see #280.
`sourcecred graph` tends to die due to lack of heap during the Git
plugin. Node defaults to ~1.76GB of heap available, which is just not
that much. This commit uses the `--max_old_space_size` argument to
`node` to increase the limit. We use a default value of `8192`, and it's
configurable by the user via a flag.
This command is only available natively in `sourcecred graph`, because
that command turns on other node processes. For commands that run in
their original process, you would need to set the value yourself.
`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
Dandelion Mané
William Chargin
Dandelion Mané