🔀 MERGE: Remove Django storage backend

[chrisjsewell]

This PR fully removes the Django storage backend and Django dependency, whilst preserving the ability to migrate all existing django profiles to the unified psql_dostore backend schema.
This is made possible, by re-writing all django database migrations as sqlachemy based alembic ones.

In doing this, it has allowed the backend storage abstraction to move away from the limitations of the Django ORM API, principally:

1. Django required that "database connection settings" be loaded, as global variables, before connecting to the database
2. Django only allowed this single, globally set, connection configuration for a database
3. Django did not allow this connection configuration to be changed once set

This thus enforced that only a single profile's storage could be accessed, for the lifetime of a Python process, i.e. no profile switching was allowed.
For sqlalchemy, no such limitation exists, since any number of sqlalchemy.Engine instances can be created, with different connection configurations.

The limitation in django also promoted some "bad practices" in aiida, namely the use of multiple global variables and their indirect reference by class methods. Global variables are problematic for numerous reasons, particularly when overused, see e.g. https://stackoverflow.com/questions/10525582/why-are-global-variables-considered-bad-practice

New UML

The following UML diagrams highlight the changes in the storage abstraction, importantly, global variables are alway accessed via the Manager and storage is always accessed via the storage backend:

Previous

New

The following changes have then been made to the abstraction:

• The BackendManager class has been fully removed: all interaction with a profile's storage now routes through the Backend class.

• The Backend class includes the class methods: version_head, version_profile and migrate, which take a Profile as input (for connection configuration). These allow for profiles with storage at an old schema revision to be assessed and migrated.

  • These methods, and all migrations, never act on or set the global profile/backend, i.e. it is perfectly possible to migrate
    the storage of a profile that is different to the globally loaded profile.
  • The Profile has a Profile.get_storage_cls, for returning the correct subclass from which to call these methods
  • There is no longer the concept of a "schema generation version"; this is not necessary, since one can simply remove past migration revisions, if they are no longer supported.

• A Backend class is instantiated with a Profile, at which point it should validate that the profile's storage is accessible and at the latest (a.k.a head) schema version

  • this includes both the database and repository validation, for example that the container ID matches the database's repository|uuid key value.

• The CURRENT_AUTOGROUP global variable has been removed and, instead, an AutogroupManager is instantiated on the storage backend, available via Backend.autogroup, which can be enabled/disabled(default).
  This ensures that a node will never try to add itself to an AutoGroup associated with a different backend.

Specific to the PsqlDosBackend (formerly SqlaBackend)

• All schema validation / migration logic goes through the PsqlDostoreMigrator class. This has somewhat similar to functionality to the previous BackendManager, except it should never be accessed directly (except for migration tests), but always via the PsqlDostoreBackend.

  • PsqlDostoreMigrator is also "exposed" for developers via python -m aiida.backends.sqlalchemy.alembic_cli, which you can use to view the migration history, and to create new migrations (after changes to the ORM)

• To support legacy profiles (pre v2.0), we maintain three alembic branches:

  • A django branch and an sqlalchemy branch, whose "heads" have identical schemas
  • A main branch, which all new migrations will build on.
  • See Starting from specific base branch, run single branch sqlalchemy/alembic#983, for some discussion on branches

• When calling PsqlDosBackend.migrate:

  1. If the database is completely empty, e.g. called via verdi setup, we simply initialise it with the current ORM schema, rather than going up through every migration (as is the case now.
  2. If a legacy django database, we transfer the version to alembic, migrate to the head of the django branch,
     reset the revision as one on the main branch, and then migrate to the head of the main branch
  3. If a legacy sqlalchemy database, we migrate to the head of the sqlalchemy branch,
     reset the revision as one on the main branch, and then migrate to the head of the main branch
  4. If the version is on the main branch, we migrate to the head of the main branch

• Because migrations are no longer tied to the global profile, this has greatly improved the test infrastructure for migrations.

  • Before, these migrations tested by taking the global profile, migrating it down to just before the target migration, adding data, then migrating back up
  • Now, the migration tests are fully isolated: A separate PGTest PostgreSQL database cluster is created when required, and scoped to the whole test session, then a separate, empty database is created/destroyed, per migration test, and a migration test simply migrates up to the target migration.
  • This means there are no issues with missing/incomplete downward migrations, and the result of the migration can never affect subsequent testing.

Contexted profile access

One key goal of this "re-abstraction", is to allow for "contexed" profile access, wich is now available using the profile_context function:

from aiida import profile_context, orm

# access a profile's stroage, without affecting the global state
with profile_context("x"):
    orm.load_node(1)

# access a different profile's storage, during the same Python session
with profile_context("y") as profile_context:
    orm.load_node(1)

One can even use an archive as a profile:

from aiida.manage import Profile

archive_profile = Profile(
    'default', {
        'storage': {
            'backend': 'archive.sqlite',
            'config': {
                'path': "/path/to/archive.aiida"
            }
        },
        'process_control': {
            'backend': 'null',
            'config': {}
        }
    }
)
with profile_context(archive_profile) as profile_context:
    orm.load_node(1)

Changes to test fixtures

There were inconsistencies and issues with the resetting of storage for AiidaTestCase and TestManager (used by the clear_database fixtures):

• AiidaTestCase had a bunch of custom code to delete and recreate the repository, but did not set the new repository UUID in the database
• TestManager did not clear the repository at all

This is fixed, by adding the Backend._clear abstract method and the PsqlDostoreBackend._clear implementation, which allows the storage to be properly reset in the tests, in a backend agnostic manner.
It has also allowed for the removal of AiidaTestImplementation/SqlAlchemyTests, since the test fixtures no longer require any backend specific code.

In line with this change, TestManager.reset_db has been deprecated and renamed to TestManager.clear_profile and also the pytest fixtures: clear_database, clear_database_after_test, clear_database_before_test and clear_database_before_test_class have been deprecated and replaced with aiida_profile_clean, aiida_profile_clean_class

---

Notes:

The large + diff on this PR, is primarily due to tests/backends/aiida_sqlalchemy/migrations/test_all_schema containing a YAML file, specifying the full database schema for all past revisions (django & sqlalchemy); tables, columns, constraints (primary key, unique, foreign key), and indexes.

[chrisjsewell]

• note to self: should improve verdi profile show, to handle the new nested structure of the config dict

[sphuber]

Thanks @chrisjsewell . A lot of work. Reviewing therefore is also not the easiest, but I gave it a real shot. Put various comments in the files that I went through and will list some other general comments here below.

Naming

I wonder if there is maybe a slightly better name for the PsqlDostore and psql_dostore abbreviations for the backend type. When I first read it, I really tripped over the store part, only realizing afterwards that it refers to disk object store. I wonder whether either going full abbreviation PsqlDos/psql_dos may be better, or alternatively PsqlDiskObjectStore/psql_disk_object_store. For the class name I prefer the latter, but for the string identifier I think psql_dos may be fine, but I wouldn't mix, so I would go for PsqlDos/psql_dos as the best option.

Generic

• For my understanding, you specify the branch_label in the aiida/backends/sqlalchemy/migrations/versions/e15ef2630a1b_initial_schema.py migration, but not in any other. I take it is only necessary to indicate this in the very first migration of a branch? Consecutive migrations will automatically inherit through the revision numbers?
• What is the purpose of aiida/backends/sqlalchemy/migrations/versions/main_0001.py? It just contains todo's for now.

Behavior

• When running an unmigrated Django profile, this is the error displayed when loading the backend (through verdi):

Critical: The database has no known version.

This should probably be the typical message saying that the current version is not compatible with that of the code and the database needs be migrated, saying to use verdi storage migrate.

• It seems that you no longer use the schema generation. Is there still a plan to decommission the very early migrations at some point? Is there a clean and efficient way to do this with alembic perhaps that doesn't require this custom concept? That would be great, but in any case I do think it is important we think about how we will do this in the future. I think it still makes sense to do it if we can do it without interruptions for users. (What is acceptable is that they cannot skip major versions when upgrading their aiida-core versions).

• verdi status no longer shows the backend schema version. Fine to remove it from here, but I think it is useful to have this somewhere for easy debugging. Maybe in verdi devel then if we think this is not necessary for users typically? Also should update deprecation warning in verdi database version then which points to verdi status as the replacement.

Codebase

Still some references of Django in the source code that may need to be removed/changed:

• In aiida/manage/configuration/schema/config-v7.schema.json the default should definitely be changed to psql_dostore.
• In aiida/manage/configuration/schema/config-v5.schema.json and aiida/manage/configuration/schema/config-v6.schema.json there is still django as the default for the backend. Should we retroactively change this to sqlalchemy? Not sure it will ever come into play but couldn't hurt, could it?
• docs/source/intro/troubleshooting.rst still references django in output of verdi status, best to replace it with something else. It is just the profile name and repository path, but would be better to have some standard profile name not connected to legacy backend.
• docs/source/internals/database.rst still describes Django database tables that will be removed auth_group etc.
• docs/source/howto/installation.rst still has old output of verdi profile show including django as backend
• ocs/source/conf.py remove workaround for Django by defining autodoc_default_options
• docs/source/nitpick-exceptions remove exceptions related to Django classes

[sphuber]

This fixture was explicitly created for a unit test of the REST API. Unless the test can be removed, this should probably not be removed.

[chrisjsewell]

the tests pass without this fixture though. What I want to understand, to think if/how it should be re-instated, is exactly what the pool_timeout=1, max_overflow=0 are intended to test, as this was not documented at all

[sphuber]

the tests pass without this fixture though

Well the point is that it is creating a situation (an engine with reduced pool timeout) that caused problems in real-life setups. The test made sure that even under those constrained conditions, things still worked. So of course if you remove the constraint it might still work, but you are not testing what you were intending to test. The settings were such to ensure that if the original problem was still there, it would be triggered as fast as possible. With more lenient settings, the test might pass simply because there is a lot of buffer, even with the underlying problem still being there.

[chrisjsewell]

I think your understanding of the fixture and test was incorrect 😬, which was my point of it being poorly documented.
The actual goal has nothing really to do with pool timeouts, it is just checking that threads properly close their database connections (see b9d4bbe).

Anyhow, I've now re-worked it in 1b98a91 (#5330).
Rather than trying to hack into the backend's connection creation, the profile's storage_config section now allows for an engine_kwargs key, which is used by create_sqlalchemy_engine to allow any additional kwargs to be passed to sqlalchemy.create_engine

[chrisjsewell]

Note, its still not ideal, because the restapi is essentially hard-coded to the psql_dos backend, relying on sqlalchemy's scoped_session to handle thread safety.
Going forward, I'm going to look into making get_manager thread local, which should mean you can properly use AiiDA in a multi-threaded mode

[sphuber]

I think your understanding of the fixture and test was incorrect grimacing,

Are you really sure. I remember the PR that added that fixture. The problem was as you say that the REST API threads weren't properly closing the database connections. This would cause them to accumulate and at some point the calls would fail since there were no more connections allowed. To test the fix, @CasperWA wanted to make sure that there was a test that would try to hit the limit. If this were to be done with the standard settings, it would take way too long, so to speed up the process, the engine was configured such that it would hit the error almost instantly in case of a regression.

[chrisjsewell]

yep thats what it does now 👍

[CasperWA]

[chrisjsewell]

Just making sure to not break your good work 😉

[chrisjsewell]

Thanks @chrisjsewell . A lot of work. Reviewing therefore is also not the easiest, but I gave it a real shot.

Cheers! Where possible, I tried to keep the diffs to a minimum 😅

naming

yep happy to discuss, your suggestions seem reasonable

For my understanding, you specify the branch_label in the aiida/backends/sqlalchemy/migrations/versions/e15ef2630a1b_initial_schema.py migration, but not in any other. I take it is only necessary to indicate this in the very first migration of a branch? Consecutive migrations will automatically inherit through the revision numbers?

Yes indeed, you can see this via: python -m aiida.backends.sqlalchemy.alembic_cli history

• What is the purpose of aiida/backends/sqlalchemy/migrations/versions/main_0001.py? It just contains todo's for now.

As discussed in sqlalchemy/alembic#983, there are now three "branches": django, sqlalchemy and main, main is where all new migrations will be "appended" on to.
The TODO is to add the full initial database schema creation (the latest one), similar to the initial revisions of the django and sqlalchemy branches.

When running an unmigrated Django profile, this is the error displayed when loading the backend (through verdi) ...

Ooops, this was just a lil bug, fixed in 9d8ab16, i.e. if there is no alembic version table, but there is a django version table, then it shows a specific error message about this

It seems that you no longer use the schema generation. Is there still a plan to decommission the very early migrations at some point?

Yes there is, you just simply remove them 😄
This does not affect users (unless their database was at those revisions obviously), since new storage is now created without going through the migrations, instead directly in PsqlDostoreMigrator.initialise.
Then we could always just make a list of the ones we remove, to provide a specific error message for databases at these revisions.

verdi status no longer shows the backend schema version. Fine to remove it from here, but I think it is useful to have this somewhere for easy debugging. Maybe in verdi devel then if we think this is not necessary for users typically? Also should update deprecation warning in verdi database version then which points to verdi status as the replacement.

Yep, very simply, you just call Backend.version_profile(profile) with whatever profile you want 😄
Or you can also run python -m aiida.backends.sqlalchemy.alembic_cli current

Still some references of Django in the source code that may need to be removed/changed:

Yep cheers. To note, I haven't touched the documentation at all, apart from to remove django references. I'll do some improvements of the developer documentation in a follow-up PR

[chrisjsewell]

• explain in top-comment that all the django schema, in tests/backends/aiida_sqlalchemy/migrations/test_all_schema were generated before this PR was applied (manually by me), then I wrote the alembic migrations to pass these regression tests. I looked into having this as an initial commit before this PR, but it was just impossible to write the necessary code in the tests to automate this (what with django's hardcoding the connection configuration)

[chrisjsewell]

verdi status no longer shows the backend schema version. Fine to remove it from here, but I think it is useful to have this somewhere for easy debugging. Maybe in verdi devel then if we think this is not necessary for users typically? Also should update deprecation warning in verdi database version then which points to verdi status as the replacement.

Added verdi storage version and updated deprecation warning

[chrisjsewell]

Interestingly, before 4fe874c, AiidaTestCase.tearDownClass was actually invalidating the storage for every subsequent test, by creating a new container without updating the repository UUID in the database

[chrisjsewell]

• CURRENT_AUTOGROUP is a bit problematic, when allowing for multiple profile contexts, i.e. you would not want to try to add nodes for one backend to an autogroup associated with another backend. This should probably be "on" the backend

[chrisjsewell]

Before f26ce67, there were inconsistencies and issues with the resetting of storage for AiidaTestCase and TestManager (used by the clear_database fixtures):

• AiidaTestCase had a bunch of custom code to delete and recreate the repository, but did not set the new repository UUID in the database
• TestManager did not clear the repository at all

This is fixed, by adding the Backend._clear abstract method and the PsqlDostoreBackend._clear implementation, which alows the storage to be properly reset in the tests, in a backend agnostic manner.
It has also allowed for the removal of AiidaTestImplementation/SqlAlchemyTests, since the test fixtures no longer require any backend specific code.

In line with this change, TestManager.reset_db has been deprecated and renamed to TestManager.clear_profile and also the pytest fixtures: clear_database, clear_database_after_test, clear_database_before_test and clear_database_before_test_class have been deprecated and replaced with aiida_profile_clean, aiida_profile_clean_class

[chrisjsewell]

In 07b2706, I have refactored the "problematic" auto-group functionality.
The CURRENT_AUTOGROUP global variable has been removed and, instead, an AutogroupManager is instantiated on the storage backend, available via Backend.autogroup, which can be enabled/disabled(default).
This ensures that a node will never try to add itself to an AutoGroup associated with a different backend.

[chrisjsewell]

In fedd4e3, I have removed the PROFILE and STORAGE_BACKEND global variables, with the global profile and its storage backend now correctly managed by the Manage instance.
This allows for the global profile to be properly "unloaded" by also closing any related resources, such as the storage (postgresql) connection and the communicator (rabbitmq) connection, and will eventually allow profile switching (still need to get rid of the SESSION_FACTORY global variable for that).

@sphuber during this, I also "fixed" your rabbitmq version check, which was forcing that a connection to be made with rabbitmq on every load_profile, as opposed to the expected lazy loading of the communicator, e.g. even unrelated commands like verdi storage info would fail if you don't have an available connection to rabbitmq.
check_rabbitmq_version now sits within Manager.create_communicator, and is also explicitly checked in verdi_status.

[chrisjsewell]

Some lower-priority TODOs I think can be left to later PRs:

• Make Profile.storage_cls select the backend class via an entry_point
• In Profile.set_storage and Profile.set_process_controller, use a classmethod on the backend class, to validate the config
• Profile.repository_path should be moved onto the backend class
  • A potential issue here though, is that repository_path is now also used for sandbox folders, which is really a different concept
• Profile.rmq_prefix and Profile.get_rmq_url should be moved to a separate "backend class for process control", similar to Profile.storage_cls
• Currently, Config.delete_profile contains logic very specific to the psql_dos, and so needs to be refactored
  • By extension, this would also require changes in verdi profile delete
  • This is just about the only code left, hard-coded to a backend (except verdi setup I guess)
• In verdi setup there is no check that the database/repository URL you supply is initially empty, e.g. is not being used by another profile
  • I don't believe this has ever been the case
  • I guess it would be a very rare corner-case, hence not a priority
• aiida/backends/sqlalchemy/alembic_cli.py::alembic_revision currently fails, because alembic.RevisionContext._run_environment has issues with multiple heads (it works if you comment out the initial auto-generate check in that method)
• Improve ArchiveReadOnlyBackend, in light of the backend abstract improvements
  • Eventually we should be able to load a Profile with e.g. the storage.backend = "archive" and the config pointing to the archive path.
• "Unify" tests/backends/aiida_sqlalchemy/test_utils.py and tests/backends/aiida_sqlalchemy/migrations/conftest.py
• aiida/restapi/common/utils.py::close_thread_connection should eventually not be hard-coded to the psql_dos backend
  • More generally, there should be a consistent mechanism for using aiida in multi-threaded mode, rather than relying on thread-local sqla sessions
  • Now that most global variables have been consolidated onto Manager, we could perhaps make this thread-local
• profile_context should probably restore any originally loaded profile, on exit

[chrisjsewell]

Still some references of Django in the source code that may need to be removed/changed:

done

[chrisjsewell]

• db_dbnode.repository_metadata is currently nullable, it should most definitely be non-nullable.
  • I feel this was essentially a minor mistake, in the django 0046 and sqla 7536a82b2cc4 migrations
  • Technically, if we are "respecting" all current migration, we need to add two migrations, one for converting all null values to empty dicts, then one to add the non-null constraint
  • Practically, all the migration are un-released (in v2), plus it's also doubtful any DB would contain a null value, so we could cheat and retroactively add this to existing migrations
  • what's your feeling on this @sphuber?

[chrisjsewell]

This is just about good to go

[sphuber]

• explain in top-comment that all the django schema, in tests/backends/aiida_sqlalchemy/migrations/test_all_schema were generated before this PR was applied (manually by me), then I wrote the alembic migrations to pass these regression tests. I looked into having this as an initial commit before this PR, but it was just impossible to write the necessary code in the tests to automate this (what with django's hardcoding the connection configuration)

Note that it still maybe very useful to create this as a commit now, after the fact. It would be very easy to rebase this and create a single commit just with the regression tests.

[sphuber]

@sphuber during this, I also "fixed" your rabbitmq version check, which was forcing that a connection to be made with rabbitmq on every load_profile, as opposed to the expected lazy loading of the communicator, e.g. even unrelated commands like verdi storage info would fail if you don't have an available connection to rabbitmq.
check_rabbitmq_version now sits within Manager.create_communicator, and is also explicitly checked in verdi_status.

This was done intentionally though. At least @giovannipizzi had requested that it would run the check as soon as the profile would be loaded. This would prevent a user potentially creating a bunch of nodes and doing some work, only to submit a workflow and find out the version is not supported. I think I am personally ok with that, so I wouldn't mind keeping your fix, but maybe @giovannipizzi should also give his ok.

[sphuber]

I feel this was essentially a minor mistake, in the django 0046 and sqla 7536a82b2cc4 migrations
Technically, if we are "respecting" all current migration, we need to add two migrations, one for converting all null values to empty dicts, then one to add the non-null constraint
Practically, all the migration are un-released (in v2), plus it's also doubtful any DB would contain a null value, so we could cheat and retroactively add this to existing migrations

I indeed initially made it nullable, but then realized it was better to always have a value, so in 4a347b6 I added the default. It is true that we probably also should have made it non-nullable at that point. Since the added default was very early, and we indeed don't support unreleased schemas, I would be fine with simply doing this change in place without any explicit migrations.

[chrisjsewell]

At least @giovannipizzi had requested that it would run the check as soon as the profile would be loaded. This would prevent a user potentially creating a bunch of nodes and doing some work, only to submit a workflow and find out the version is not supported.

Yeh I'm strongly against that. If we really want to prevent such things, I would even suggest actually raising an exception when trying to load the communicator, if RabbitMQ is the wrong version, with a configuration option to deactivate this, for users that still want to proceed.

[giovannipizzi]

At least @giovannipizzi had requested that it would run the check as soon as the profile would be loaded. This would prevent a user potentially creating a bunch of nodes and doing some work, only to submit a workflow and find out the version is not supported.

As long as the message appears when the user first runs a workflow (rather than whet they first create nodes), I'm still OK as the problems will occur when running.
However, in the current change, when/where will the error occur? In the shell/notebook/verdi run being run, or only in the daemon and show in the daemon log? If it's the former (i.e., the user sees it) then I'm OK.

Yeh I'm strongly against that. If we really want to prevent such things, I would even suggest actually raising an exception when trying to load the communicator, if RabbitMQ is the wrong version, with a configuration option to deactivate this, for users that still want to proceed.

Just to be clear, you are against early checks?
Thinking better to it now, maybe indeed not having an early check is OK (I guess one could install AiiDA and, with your new implementations, inspect the content of an archive without having to even install a DB or RabbitMQ, right?), as long as the error is visible to users. OK to even raise if it's possible to disable it for experienced users (e.g. with a new RMQ version that they configure appropriately).

What do you think @sphuber and @chrisjsewell ?

[chrisjsewell]

inspect the content of an archive without having to even install a DB or RabbitMQ, right?

exactly 👍 the check is definitely a good thing, and appreciated, but not at the expense of all other aiida functionality

[sphuber]

Just to be clear, you are against early checks?

I am not against early checks per-se, only if that has other disadvantages. I have to agree with @chrisjsewell that for some parts of AiiDA, connection with RabbitMQ is not necessary and making it fail even when not necessary, seems unnecessarily restrictive.

I think the current solution is better. It will only perform the check and warn when a communicator is created, which is whenever you start to run any processes. For example, running verdi shell won't show anything, but then you get the following:

In [1]: from aiida.engine import calcfunction

In [2]: @calcfunction
   ...: def add(a, b):
   ...:     return a + b
   ...: 

In [3]: add(Int(1), Int(2))
Warning: RabbitMQ v3.8.2 is not supported and will cause unexpected problems!
Warning: It can cause long-running workflows to crash and jobs to be submitted multiple times.
Warning: See https://github.com/aiidateam/aiida-core/wiki/RabbitMQ-version-to-use for details.
Out[3]: <Int: uuid: ebdec2dc-53dd-4ae2-af0b-727d91de5cbb (pk: 342) value: 3>

OK to even raise if it's possible to disable it for experienced users (e.g. with a new RMQ version that they configure appropriately).

I don't think I would go for turning it into an exception for now. Would leave it as a warning

[chrisjsewell]

Plus, it also shows in verdi status:

[sphuber]

Alright, I am ready to give this my seal of approval. We have done quite some testing on the migrations and I think they are now ready to be tested by the rest of the team. So

catch ya later Django!

[chrisjsewell]

@sphuber, geez I'm in the middle of re-writing the PR description which, as you requested, was gonna be added to the commit description.
Let a man merge he's own PR 😒

(not the end of the world, but please never merge my PRs)

[chrisjsewell]

Thanks for the review though!

[sphuber]

Apologies, since you pushed the rebased branch I figured you didn't want to include the commit message. Since there has been a lot of work already, I wasn't going to push it and so thought to get on with it. I didn't see why you would push already if you were still going to work on the commit message. A quick note that you were still going to do that would have done the trick.

[chrisjsewell]

I didn't see why you would push already if you were still going to work on the commit message.

God works in mysterious ways 👼

Added my beautiful UML diagrams to the description, which anyhow would not have gone in the commit message

[chrisjsewell]

@sphuber not sure if I've broken codecov integration; they are definitely triggering https://app.codecov.io/gh/aiidateam/aiida-core/compare/5330, but not seeing the results "pushed back"

[sphuber]

@sphuber not sure if I've broken codecov integration; they are definitely triggering https://app.codecov.io/gh/aiidateam/aiida-core/compare/5330, but not seeing the results "pushed back"

Looks like it. It is indeed no longer showing up on the pull requests. But could also be unrelated and just a problem between Github and Codecov.

[chrisjsewell]

@sphuber its ok now, I reset the webhook