Skip to content
New issue

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

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

Already on GitHub? # to your account

feat(gadget-sdk): improve MultiJobRunner builder #382

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

Serial-ATA
Copy link
Contributor

Jobs can be built using tuples now. Some comparisons:

Default price targets

Old

MultiJobRunner::new(&env)
    .with_job()
    .with_default_price_targets()
    .finish(x_square)

New

MultiJobRunner::new(&env)
    .job(x_square)

Custom price targets

Old

use tangle_subxt::tangle_testnet_runtime::api::runtime_types::tangle_primitives::services::PriceTargets;

let price_targets = PriceTargets {
    cpu: 5,
    mem: 10,
    storage_hdd: 15,
    storage_ssd: 20,
    storage_nvme: 25
};

MultiJobRunner::new(&env)
    .with_job()
    .with_price_targets(price_targets)
    .finish(x_square)

New

use tangle_subxt::tangle_testnet_runtime::api::runtime_types::tangle_primitives::services::PriceTargets;

let price_targets = PriceTargets {
    cpu: 5,
    mem: 10,
    storage_hdd: 15,
    storage_ssd: 20,
    storage_nvme: 25
};

MultiJobRunner::new(env)
    .job((x_square2, price_targets))

closes #371

@Serial-ATA Serial-ATA marked this pull request as ready for review October 23, 2024 18:34
@Serial-ATA Serial-ATA requested review from drewstone and tbraun96 and removed request for drewstone October 23, 2024 18:35
Copy link
Contributor

@shekohex shekohex left a comment

Choose a reason for hiding this comment

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

Looks good.

/// This provides a [`Default`] impl for a zeroed-out [`PriceTargets`].
///
/// [`PriceTargets`]: tangle_subxt::tangle_testnet_runtime::api::runtime_types::tangle_primitives::services::PriceTargets
pub struct PriceTargets(TangleSubxtPriceTargets);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can provide a default implementation in subxt and remove this wrapper. But it is good for now.

@tbraun96
Copy link
Contributor

Just my $0.02. The tuple notation is awkward to me simply because I haven't really seen it in builder notation often and isn't intuitive.

How about a compromise:

let price_targets = PriceTargets {
    cpu: 5,
    mem: 10,
    storage_hdd: 15,
    storage_ssd: 20,
    storage_nvme: 25
};

MultiJobRunner::new(env)
    .job(x_square)
    .with_price_targets(price_targets)
    .job(x_square2)
    .job(x_square3)
    .with_default_price_targets()
    .job(s_square4)
    [...]
    .run()
    .await?

@Serial-ATA
Copy link
Contributor Author

@tbraun96 the tuples are just for convenience, they actually get converted to JobBuilders.

let price_targets = PriceTargets {
    cpu: 5,
    mem: 10,
    storage_hdd: 15,
    storage_ssd: 20,
    storage_nvme: 25
};

// These two are equivalent

MultiJobRunner::new(env)
    .job((x_square2, price_targets))

MultiJobRunner::new(env)
    .job(JobBuilder::new(x_square2).price_targets(price_targets))

// As are these

MultiJobRunner::new(env)
    .job(x_square)

MultiJobRunner::new(env)
    .job(JobBuilder::new(x_square))

@tbraun96
Copy link
Contributor

@tbraun96 the tuples are just for convenience, they actually get converted to JobBuilders.

let price_targets = PriceTargets {
    cpu: 5,
    mem: 10,
    storage_hdd: 15,
    storage_ssd: 20,
    storage_nvme: 25
};

// These two are equivalent

MultiJobRunner::new(env)
    .job((x_square2, price_targets))

MultiJobRunner::new(env)
    .job(JobBuilder::new(x_square2).price_targets(price_targets))

// As are these

MultiJobRunner::new(env)
    .job(x_square)

MultiJobRunner::new(env)
    .job(JobBuilder::new(x_square))

Oh cool

@drewstone
Copy link
Contributor

drewstone commented Oct 24, 2024

@tbraun96 I'm still surprised how you think this makes sense

MultiJobRunner::new(env)
    .job(x_square)
    .with_price_targets(price_targets)
    .job(x_square2)
    .job(x_square3)
    .with_default_price_targets()
    .job(s_square4)
    [...]
    .run()
    .await?

What are the price targets for the jobs? Is it the price target after it's specified? What about x_square2 would it be default? This is confusing lol. The tupleness is still 100% clearer on what is being attributed to what.

@drewstone
Copy link
Contributor

@Serial-ATA @Tjemmmic we should meet or you two should to discuss ensuring the Eigenlayer runner is also made compatible with this system.

@drewstone drewstone merged commit e9c0785 into main Oct 24, 2024
12 checks passed
@drewstone drewstone deleted the serial/multijobrunner-builder branch October 24, 2024 07:14
This was referenced Oct 24, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Completed ✅
Development

Successfully merging this pull request may close these issues.

[TASK] Follow the traditional builder pattern in MultiJobRunner
4 participants