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

Fixes for lotus-provider, and conveniences #11419

Merged
merged 13 commits into from
Nov 21, 2023
Merged

Conversation

snadrus
Copy link
Collaborator

@snadrus snadrus commented Nov 15, 2023

Related Issues

FilOzone/Space-Panthers#11

Proposed Changes

  • make install now installs lotus-provider
  • configuration provides more defaults
  • cli tester for windowpost

Additional Info

For testing, these top-level commands (flags available):
lotus-provider test window-post here
Good for: CLI testing to ensure configuration layers & basic functionality work as expected.
Does not post to chain.

lotus-provider test window-post task
Good for: E2E testing of a lotus-provider that's enabled for the window-post task.
Does not post to chain.

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@snadrus snadrus requested a review from a team as a code owner November 15, 2023 01:02
@snadrus snadrus changed the title High-demand lotus-provider conveniences Fixes for lotus-provider, and conveniences Nov 15, 2023
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Some nits / Qs

lib/harmony/resources/resources.go Outdated Show resolved Hide resolved
provider/lpwindow/compute_task.go Outdated Show resolved Hide resolved
},
}

var provingComputeWindowPoStCmd = &cli.Command{
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to have a way to submit a mock-compute task to the database, so the computation isn't done in the local cli, but on the real workers (which imo is what SPs will expect in most cases - running check tasks on the cluster with proper scheduling, but no submitting things to the chain)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that makes sense. Would you want it to do that with live updates, or just direct them to logs and database to evaluate the success?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have either this or a separate command poll for success

@rjan90
Copy link
Contributor

rjan90 commented Nov 15, 2023

It would be nice to add these to the default config, as they are needed for getting lotus-provider configured, but are currently missing from the basic config.toml:

[Addresses]
MinerAddresses = [ "MyMinerID"]

[Apis]
ChainApiInfo = ["myJWT"]

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Some more problems here

cmd/lotus-provider/run.go Outdated Show resolved Hide resolved
cmd/lotus-provider/run.go Outdated Show resolved Hide resolved
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

This is objectively better, just two small things to fix

cmd/lotus-provider/run.go Show resolved Hide resolved
node/builder_chain.go Outdated Show resolved Hide resolved
provider/lpmessage/sender.go Outdated Show resolved Hide resolved
@magik6k
Copy link
Contributor

magik6k commented Nov 20, 2023

(also some conflicts)

@snadrus
Copy link
Collaborator Author

snadrus commented Nov 21, 2023

I'm going to propose a different CLI:

lotus-provider test window-post task
and
lotus-provider test window-post here

That is a bit clearer on what's going on with the two test commands, and provides an avenue for future integration test behavior.

As a side-note, I also made it possible to have titled sql files since I was starting to get lost in all of them. I didn't persist the titles to the database because that seems pointless as it's a developer convenience that doesn't really have an analog at runtime.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

1 think likely to fix, but this PR does make a lot of things better enough to merge now

provider/lpwindow/compute_task.go Show resolved Hide resolved
@magik6k magik6k merged commit aa2640d into feat/sturdypost Nov 21, 2023
84 of 89 checks passed
@magik6k magik6k deleted the fix-sturdy-tests branch November 21, 2023 22:24
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.

3 participants