Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ✨ covalent resource and asset #27

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

DistributedDoge
Copy link
Collaborator

@DistributedDoge DistributedDoge commented Jan 5, 2024

Built an asset that fetches all mainnet project registry transactions. Appears to be working locally or inside reconfigured forked CI .

Manual config changes needed for new CI workflow:

  • repository wide secret COVALENT_API_KEY set up in here.

New asset fetches ~3k transactions, which should cost about 300 covalent credits out of 100k monthly free tier.
Takes about 3 minutes to run, but in parallel with other assets doesn't affect overall speed to much.

Copy link
Owner

@davidgasquez davidgasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR! Excited to have Covalent data around.

Left a few comments afteer a super quick review. Feel free to ping me once this is ready for a more formal one. 😊

New asset fetches ~3k transactions, which should cost about 300 covalent credits out of 100k monthly free tier.

Nice! Seems we can rely a lot more on Covalent.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ggdp/resources.py Outdated Show resolved Hide resolved
@davidgasquez
Copy link
Owner

What is needed for this PR to be ready? Happy to merge it even if not fully complete!

@DistributedDoge
Copy link
Collaborator Author

DistributedDoge commented Jan 10, 2024

The only thing needed from your side:

  • repository-wide secret Covalent API key '${{ secrets.COVALENT_KEY }}'.

@DistributedDoge
Copy link
Collaborator Author

If you want you can consider it ready to merge. Tested new version on the fork, and CI appears to be working, as long as COVALENT_KEY is set up first.

https://github.com/DistributedDoge/gitcoin-grants-data-portal/actions/runs/7485200208/job/20373252823

New behaviour:

  • not having a key at all will result in Error when trying make run or fetching assets in make dev.
  • having any random string as API key should materialize other assets except for this one
  • dagster run locally expects COVALENT_API_KEY, CI expects COVALENT_KEY
  • dagster throws helpful error message to let you know what it is missing though:
dagster._config.errors.PostProcessingError: You have attempted to fetch the environment variable "COVALENT_API_KEY" which is not set. In order for this execution to succeed it must be set in this environment.

Problems:

  • I could not find a way to make asset optional if triggered from asset job selecting [*] like we do in make run.
  • DESIRED behaviour would be that if we don't have a key set up, an asset doesn't even try to materialize

I was hoping asset groups would be a solution, alas I don't think dagster asset selection language used by CLI and python is actually using asset groups at all.

@asset(
    compute_kind="Covalent_API",
    group_name="chain_data",
)

@DistributedDoge DistributedDoge marked this pull request as ready for review January 11, 2024 07:18
@DistributedDoge DistributedDoge changed the title [WIP] feat: ✨ covalent resource and asset feat: ✨ covalent resource and asset Jan 11, 2024
@DistributedDoge
Copy link
Collaborator Author

DistributedDoge commented Jan 11, 2024

CI run is actually failing. Back to the drawing board, strange that I didn't see this error maybe your covalent key wasn't pasted right?

dagster._core.errors.DagsterExecutionStepExecutionError: Error occurred while executing op "ethereum_project_registry_tx"::

requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://api.covalenthq.com/v1/eth-mainnet/address/0x03506eD3f57892C85DB20C36846e9c808aFe9ef4/transactions_v3/page/1/

Copy link
Owner

@davidgasquez davidgasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for tidying this up!

Left a couple of mostly style suggestions.

I could not find a way to make asset optional if triggered from asset job selecting [*] like we do in make run.

This is something we could ask Dagster folks. We need (1) a way to select based on tags for this and the future incremental approach.

DESIRED behaviour would be that if we don't have a key set up, an asset doesn't even try to materialize

We can do this! Right now, the asset relies on the io manager and expect a DataFrame. We should move out of that and return a MaterializedResult we can control better.

https://github.com/davidgasquez/filecoin-data-portal/blob/main/fdp/assets.py#L16-L35

The asset code will do the same, but we add the step of materializing to DuckDB if the request was succesful.

Does this makes sense?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/run.yml Outdated Show resolved Hide resolved
ggdp/__init__.py Outdated Show resolved Hide resolved
ggdp/assets.py Outdated Show resolved Hide resolved
@davidgasquez
Copy link
Owner

strange that I didn't see this error maybe your covalent key wasn't pasted right?

I still haven't added the Covalent API key. Doing that right now!

Co-authored-by: David Gasquez <davidgasquez@gmail.com>
@davidgasquez
Copy link
Owner

Alright, COVALENT_API_KEY is now on the CI environment! Sorry for the delay there!

Co-authored-by: David Gasquez <davidgasquez@gmail.com>
@DistributedDoge
Copy link
Collaborator Author

If you could kindly squash before merging, or however Github calls it. I don't like leaving a mess in commit history.

@davidgasquez
Copy link
Owner

Yes! In fact, let me make squashing the default in this repository.

@davidgasquez davidgasquez linked an issue Jan 11, 2024 that may be closed by this pull request
3 tasks
@DistributedDoge
Copy link
Collaborator Author

Forked CI with my own API key inserted will succeed now, so the most likely cause of error here is environment mismatch between our versions.

image

Absolutely on rush to do so, but if you do happen to circle back here after the weekend:

  • make sure that the key is defined in secrets tab not variables
  • key is called COVALENT_API_KEY
  • key is pasted as is, i.e. cqt_something without any " quotes or anything

@davidgasquez
Copy link
Owner

Weird, there is actually a COVALENT_API_KEY in the repo since last week.

Going to merge and see if it needs to update the CI on main for some reason.

@davidgasquez davidgasquez merged commit f3411b0 into davidgasquez:main Jan 15, 2024
1 check failed
@DistributedDoge DistributedDoge deleted the covcontrib branch January 16, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull infrequent Transactions from Covalent API
2 participants