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

chore(blockifier): add ContractClassManagerConfig #2092

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware marked this pull request as ready for review November 17, 2024 07:44
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 47.76119% with 35 lines in your changes missing coverage. Please review.

Project coverage is 59.45%. Comparing base (e3165c4) to head (ea70b8f).
Report is 611 commits behind head on main.

Files with missing lines Patch % Lines
crates/blockifier/src/blockifier/config.rs 0.00% 30 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs 78.26% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2092       +/-   ##
===========================================
+ Coverage   40.10%   59.45%   +19.35%     
===========================================
  Files          26      151      +125     
  Lines        1895    18359    +16464     
  Branches     1895    18359    +16464     
===========================================
+ Hits          760    10916    +10156     
- Misses       1100     6742     +5642     
- Partials       35      701      +666     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 8120b6b to 055b750 Compare November 17, 2024 08:36
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 055b750 to 8d408e8 Compare November 17, 2024 09:53
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 8d408e8 to 727fdd8 Compare November 17, 2024 10:59
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/test_utils/transfers_generator.rs line 55 at r2 (raw file):

            recipient_generator_type: RECIPIENT_GENERATOR_TYPE,
            concurrency_config: ConcurrencyConfig::create_for_testing(false),
            run_native: true,

Suggestion:

false

crates/blockifier/src/test_utils/transfers_generator.rs line 86 at r2 (raw file):

        let executor_config = TransactionExecutorConfig {
            concurrency_config: config.concurrency_config.clone(),
            run_native: true,

Suggestion:

            run_native: config.run_native,

crates/native_blockifier/src/py_block_executor_test.rs line 42 at r2 (raw file):

    let mut block_executor = PyBlockExecutor::create_for_testing(
        PyConcurrencyConfig::default(),
        true, // TODO(AvivG): Default value should be different?

Suggestion:

        false, // TODO(AvivG): Default value should be different?

crates/native_blockifier/src/py_block_executor_test.rs line 123 at r2 (raw file):

    let mut block_executor = PyBlockExecutor::native_create_for_testing(
        Default::default(),
        true, // TODO(Aviv): Default value should be different?

Suggestion:

        false, // TODO(Aviv): Default value should be different?

crates/blockifier/src/blockifier/config.rs line 26 at r2 (raw file):

        TransactionExecutorConfig {
            concurrency_config: ConcurrencyConfig::default(),
            run_native: true,

We do not want it to run by default until the feature is ready.

Suggestion:

            run_native: false,

crates/blockifier/src/blockifier/config.rs line 37 at r2 (raw file):

            "run_native",
            &self.run_native,
            "Flag to enable or disable the use of native execution.",

Suggestion:

            "Enables Cairo native execution.",

Copy link
Contributor

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @Yoni-Starkware)

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avivg-starkware and @Yoni-Starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 727fdd8 to 0cc9e33 Compare November 18, 2024 15:01
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/blockifier/config.rs line 26 at r2 (raw file):

Previously, avi-starkware wrote…

We do not want it to run by default until the feature is ready.

Done.


crates/blockifier/src/test_utils/transfers_generator.rs line 55 at r2 (raw file):

            recipient_generator_type: RECIPIENT_GENERATOR_TYPE,
            concurrency_config: ConcurrencyConfig::create_for_testing(false),
            run_native: true,

Done.


crates/blockifier/src/test_utils/transfers_generator.rs line 86 at r2 (raw file):

        let executor_config = TransactionExecutorConfig {
            concurrency_config: config.concurrency_config.clone(),
            run_native: true,

Done.


crates/native_blockifier/src/py_block_executor_test.rs line 42 at r2 (raw file):

    let mut block_executor = PyBlockExecutor::create_for_testing(
        PyConcurrencyConfig::default(),
        true, // TODO(AvivG): Default value should be different?

Done.


crates/native_blockifier/src/py_block_executor_test.rs line 123 at r2 (raw file):

    let mut block_executor = PyBlockExecutor::native_create_for_testing(
        Default::default(),
        true, // TODO(Aviv): Default value should be different?

Done.


crates/blockifier/src/blockifier/config.rs line 37 at r2 (raw file):

            "run_native",
            &self.run_native,
            "Flag to enable or disable the use of native execution.",

Done.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 0cc9e33 to 944c0dc Compare November 18, 2024 15:07
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @Yoni-Starkware)


crates/native_blockifier/src/py_block_executor_test.rs line 42 at r2 (raw file):

Previously, avivg-starkware wrote…

Done.

Remove the TODO as well.


crates/native_blockifier/src/py_block_executor_test.rs line 123 at r2 (raw file):

Previously, avivg-starkware wrote…

Done.

Remove the TODO as well.

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @Yoni-Starkware)

Copy link
Contributor

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avivg-starkware and @Yoni-Starkware)


crates/blockifier/src/blockifier/config.rs line 10 at r3 (raw file):

pub struct TransactionExecutorConfig {
    pub concurrency_config: ConcurrencyConfig,
    pub run_native: bool,

Let's change the name of the flag everywhere.
The word native by itself is confusing because it is sometimes used to refer to the native_blockifier crate.

Suggestion:

    pub run_cairo_native: bool,

config/sequencer/default_config.json line 110 at r3 (raw file):

    "description": "Flag to enable or disable the use of native execution.",
    "privacy": "Public",
    "value": true

Notice that the json was not affected by your changes of config.rs

Code quote:

    "description": "Flag to enable or disable the use of native execution.",
    "privacy": "Public",
    "value": true

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 944c0dc to 39fb4fa Compare November 19, 2024 09:30
Copy link

Artifacts upload triggered. View details here

@avi-starkware
Copy link
Contributor

crates/native_blockifier/src/py_block_executor.rs line 127 at r4 (raw file):

    pub block_compilation: bool,
    pub global_contract_cache_size: usize,
}

What does block_compilation mean?
Why add the global_contract_cache_size here?
We are going to have 4 caches and a queue. All with size limits. Shouldn't we wait for the struct to be finalized before deciding on the config for it?

Code quote:

pub struct MeshiAviConfig {
    pub run_cairo_native: bool,
    pub block_compilation: bool,
    pub global_contract_cache_size: usize,
}

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 39fb4fa to e0c2f76 Compare November 19, 2024 13:17
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 9 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/blockifier/config.rs line 10 at r3 (raw file):

Previously, avi-starkware wrote…

Let's change the name of the flag everywhere.
The word native by itself is confusing because it is sometimes used to refer to the native_blockifier crate.

Done.


crates/blockifier/src/state/global_cache.rs line 17 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Yes

Done.


crates/native_blockifier/src/py_block_executor_test.rs line 42 at r2 (raw file):

Previously, avi-starkware wrote…

Remove the TODO as well.

Done.


crates/native_blockifier/src/py_block_executor_test.rs line 123 at r2 (raw file):

Previously, avi-starkware wrote…

Remove the TODO as well.

Done.


config/sequencer/default_config.json line 110 at r3 (raw file):

Previously, avi-starkware wrote…

Notice that the json was not affected by your changes of config.rs

I see thanks

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from a01c092 to add096c Compare November 20, 2024 15:18
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from add096c to 83a812d Compare November 20, 2024 15:48
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 83a812d to b2cb423 Compare November 20, 2024 15:51
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from b2cb423 to 023a2bc Compare November 20, 2024 15:57
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/blockifier/config.rs line 70 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

block_on_native_compilation/wait_on_native_compilation

Done.


crates/blockifier/src/blockifier/config.rs line 68 at r5 (raw file):

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
pub struct CairoNativeConfig {

Done.


crates/blockifier/src/blockifier/config.rs line 71 at r5 (raw file):

    pub run_cairo_native: bool,
    pub block_compilation: bool,
    pub global_contract_cache_size: usize,

Done.

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r4, 1 of 6 files at r5, 1 of 2 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 023a2bc to e66aeb8 Compare November 20, 2024 16:05
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r6, 3 of 4 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from e66aeb8 to 95c3845 Compare November 27, 2024 10:58
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware changed the title chore(blockifier): add flag run_native chore(blockifier): add ContractClassManagerConfig Nov 27, 2024
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from 95c3845 to e624709 Compare November 27, 2024 12:51
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_flag_run_native branch from e624709 to ea70b8f Compare November 27, 2024 16:31
Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r7, 1 of 1 files at r8, 2 of 2 files at r9.
Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @meship-starkware and @Yoni-Starkware)

Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware merged commit bbd1aef into main Nov 27, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2024
@avivg-starkware avivg-starkware deleted the avivg/blockifier/add_flag_run_native branch December 1, 2024 09:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants