Feature/graphman test run #3079

sduchesneau · 2021-12-17T03:35:06Z

graphman test-run

add graphman test-run command (implements stop-block mechanism to stop when the subgraph's end block is reached)

(github implementation pushed to next PR)

sduchesneau · 2021-12-17T03:36:24Z

Requires some discussion about how the comparison tests should be run. Maybe they could be hooked to releases (not build graphman in the action to make it faster), maybe they could be in a different repository, maybe they could start a bigger matrix of runs, in parallel.

node/src/manager/commands/test_run.rs

This PR is a first step in merging #3079. In this upcoming, we had copied a bunch of methods from `main.rs`, mainly those around instantiating elements to connect to chains and prepare them. In this PR, we moved - `create_ipfs_clients` - `create_ethereum_networks` (formely `create_networks`) - `create_firehose_networks` - `connect_ethereum_networks` - `connect_firehose_networks` To a common module `chain` under `graph-node` crate. There is no functionality changed in this PR, only moving code around. Small differences are the renamed of `create_networks` and its `config` argument that is now received as a reference to `Config` (instead of a owned `Config`).

* graph-node: factorize some methods inside a shared component This PR is a first step in merging #3079. In this upcoming, we had copied a bunch of methods from `main.rs`, mainly those around instantiating elements to connect to chains and prepare them. In this PR, we moved - `create_ipfs_clients` - `create_ethereum_networks` (formely `create_networks`) - `create_firehose_networks` - `connect_ethereum_networks` - `connect_firehose_networks` To a common module `chain` under `graph-node` crate. There is no functionality changed in this PR, only moving code around. Small differences are the renamed of `create_networks` and its `config` argument that is now received as a reference to `Config` (instead of a owned `Config`).

.github/workflows/sf.yml

* graph-node: factorize some methods inside a shared component This PR is a first step in merging #3079. In this upcoming, we had copied a bunch of methods from `main.rs`, mainly those around instantiating elements to connect to chains and prepare them. In this PR, we moved - `create_ipfs_clients` - `create_ethereum_networks` (formely `create_networks`) - `create_firehose_networks` - `connect_ethereum_networks` - `connect_firehose_networks` To a common module `chain` under `graph-node` crate. There is no functionality changed in this PR, only moving code around. Small differences are the renamed of `create_networks` and its `config` argument that is now received as a reference to `Config` (instead of a owned `Config`).

sduchesneau · 2022-01-10T14:36:01Z

@mangas ^ I've just removed the Github Actions part of this PR (by force-pushing rebased version over my feature-branch)
This here is only the implementation of graphman 'test-run' command, allowing the following:

bin/graphman --config eth-mainnet-firehose.toml test-run eth-mainnet {IPFS_HASH} {STOP_BLOCK}

mangas · 2022-01-10T14:40:27Z

node/src/bin/manager.rs

@@ -582,6 +605,30 @@ async fn main() {
                sleep,
            )
        }
+        TestRun {


Any objection to calling this Run and having stop block as an optional flag?

I just see the possible confusion about the purpose of this (the purpose is to do a test run, no to run production on this! it does not work in parallel with a graph-node writing on the same database, etc.)
@maoueh ?

I'm with @mangas, graphman run feels better.

About your concerns @sduchesneau , graphman is a tool that is deemed for "management of graph-node" instances, so I don't think it will be confused as production means to run a graph node.

@lutter What do you think about this, do you think there could be some confusions if we use graphman run?

I'm +1 on run, but we can keep the stop block as a required flag if that's how it's meant to be used

I am also +1 on run - I think we should just say in the help message that this is for test purposes only. Another option would be to call this exec which to me already sounds a bit more dangerous than run

mangas · 2022-01-10T14:42:02Z

Adding @leoyvens for review as well as I don't really trust my reviews yet 😄

chain/ethereum/src/codec.rs

…r on - Copied a bunch of stuff that can be shared with `main.rs` - Can refactor even more `main.rs` to be shared with stuff made for `test-run` - Implement some form of stop mechanism (block_count, stop block, time based, etc)

lutter · 2022-01-10T23:48:12Z

core/src/subgraph/instance_manager.rs

+                }
+                _ => {}
+            }
+


It seems a little weird that we stop after getting the block but before processing it - that way, the last block we'll actually process (and have entities for) will be stop_block - 1. It's probably fine, but will need to be documented in graphman run

The instance manager stops after seeing (and before processing) the block HIGHER THAN the stop block:
if block_ptr.number > stop_block

So the behavior is to process up to stop_block INCLUSIVE, as you would expect (well, it Works on My Machine ™️)

lutter · 2022-01-10T23:49:03Z

node/src/bin/manager.rs

+        subgraph: String,
+
+        /// Number of block to process
+        stop_block: i32,


This needs to say that the latest entities we'll have in the database will be for stop_block - 1.

not needed if my previous claim is right 😃

lutter · 2022-01-10T23:55:21Z

node/src/bin/manager.rs

@@ -582,6 +605,30 @@ async fn main() {
                sleep,
            )
        }
+        TestRun {


I am also +1 on run - I think we should just say in the help message that this is for test purposes only. Another option would be to call this exec which to me already sounds a bit more dangerous than run

lutter · 2022-01-11T00:00:30Z

node/src/bin/manager.rs

@@ -411,6 +434,10 @@ impl Context {
        pools
    }

+    async fn store_builder(self) -> StoreBuilder {
+        StoreBuilder::new(&self.logger, &self.node_id, &self.config, self.registry).await
+    }


Heh .. the whole point of this Context struct was so that we wouldn't have to pass the StoreBuilder around because that obscures what exactly a command might be doing with the store. It would be better to add a utility method to the Context that returns the NetworkStore, the primary pool, and the chain head listener and pass those into commands::run

Also, StoreBuilder::new will run migrations - avoiding that is a big reason why there are all these other methods on StoreBuilder so that using the wrong graphman version won't screw up the database schema.

I would change this method to call Self::store_and_primary and then copy the few lines that create a ChainHeadUpdateListener from StoreBuilder::new to here

We changed it exactly to be able to run ... migrations :D I notice the differences indeed. Migrations was required first for CI since it always starts fresh but it's also quite good when using it for development purposes as I often starts from a fresh database.

The best will be to discuss that over a video chat to see what you be the best course of action here. Two things come to mind:

Run only on pristine database

Run only when specific flag is provided

@lutter Need to chat more about this, indeed, we are using this command mostly on a completely empty database (in dev/test flow), so we need all the migrations bells and whistles...

* graph-node: factorize some methods inside a shared component This PR is a first step in merging #3079. In this upcoming, we had copied a bunch of methods from `main.rs`, mainly those around instantiating elements to connect to chains and prepare them. In this PR, we moved - `create_ipfs_clients` - `create_ethereum_networks` (formely `create_networks`) - `create_firehose_networks` - `connect_ethereum_networks` - `connect_firehose_networks` To a common module `chain` under `graph-node` crate. There is no functionality changed in this PR, only moving code around. Small differences are the renamed of `create_networks` and its `config` argument that is now received as a reference to `Config` (instead of a owned `Config`).

maoueh marked this pull request as draft December 17, 2021 03:55

maoueh reviewed Dec 17, 2021

View reviewed changes

node/src/manager/commands/test_run.rs Outdated Show resolved Hide resolved

sduchesneau force-pushed the feature/graphman-test-run branch from cc0a767 to e21a011 Compare December 20, 2021 20:45

maoueh mentioned this pull request Dec 20, 2021

graph-node: factorize some methods inside a shared component #3093

Merged

maoueh force-pushed the feature/graphman-test-run branch from 8de8594 to 56b602f Compare December 22, 2021 04:09

maoueh changed the title ~~Feature/graphman test run (IN PROGRESS)~~ Feature/graphman test run Dec 22, 2021

maoueh reviewed Dec 22, 2021

View reviewed changes

.github/workflows/sf.yml Outdated Show resolved Hide resolved

maoueh temporarily deployed to eth-mainnet December 22, 2021 04:16 Inactive

maoueh marked this pull request as ready for review January 6, 2022 17:15

evaporei mentioned this pull request Jan 6, 2022

Add stop block environment variable #3033

Closed

sduchesneau force-pushed the feature/graphman-test-run branch from b6640dc to 2a1b0c4 Compare January 10, 2022 13:28

mangas reviewed Jan 10, 2022

View reviewed changes

mangas requested a review from leoyvens January 10, 2022 14:40

mangas reviewed Jan 10, 2022

View reviewed changes

chain/ethereum/src/codec.rs Outdated Show resolved Hide resolved

Matthieu Vachon and others added 5 commits January 10, 2022 16:25

lint with 'cargo fmt --all'

63b1e75

fix code breaks from rebase with master

cd0f00c

more linting

78d1dcc

Fixed migrations not being applied

3c247c1

maoueh force-pushed the feature/graphman-test-run branch from 3b93149 to 3c247c1 Compare January 10, 2022 21:26

lutter reviewed Jan 11, 2022

View reviewed changes

rename graphman 'test-run' to 'run'

855c4b9

mangas approved these changes Jan 13, 2022

View reviewed changes

mangas merged commit 2f8bc1d into graphprotocol:master Jan 14, 2022

neysofu mentioned this pull request Sep 30, 2022

Code reuse between graphman and graph-node #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/graphman test run #3079

Feature/graphman test run #3079

sduchesneau commented Dec 17, 2021 •

edited

Loading

sduchesneau commented Dec 17, 2021

sduchesneau commented Jan 10, 2022

mangas Jan 10, 2022

sduchesneau Jan 10, 2022

maoueh Jan 10, 2022

leoyvens Jan 10, 2022 •

edited

Loading

lutter Jan 10, 2022

mangas commented Jan 10, 2022 •

edited

Loading

lutter Jan 10, 2022

sduchesneau Jan 12, 2022

lutter Jan 10, 2022

sduchesneau Jan 12, 2022

lutter Jan 10, 2022

lutter Jan 11, 2022

maoueh Jan 11, 2022

sduchesneau Jan 12, 2022

Feature/graphman test run #3079

Feature/graphman test run #3079

Conversation

sduchesneau commented Dec 17, 2021 • edited Loading

graphman test-run

sduchesneau commented Dec 17, 2021

sduchesneau commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoyvens Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangas commented Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sduchesneau commented Dec 17, 2021 •

edited

Loading

leoyvens Jan 10, 2022 •

edited

Loading

mangas commented Jan 10, 2022 •

edited

Loading