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
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
William Chargin
Dandelion Mané
Dandelion Mané