Asv v2 s3 tests (Refactored)

[grusev]

Reference Issues/PRs

Contains refactored framework for setting up shared storages + tests for AWS S3 storage

Merged 3 Prs into one:

• Additional asv tests #2185
• ASV framework refactor proposal #2227
• Comparison of memory efficiency LMDB, S3, Pandas for read/write operations on dataframes (new approach on making sure we accurately measure peakmem with ASV) #2204

Important: the benchmark tests down in this PR cannot run successfully. Therefore do not take them as criteria. All tests need to be run manually. Here are runs from 27-march:
LMDB set: https://github.com/man-group/ArcticDB/actions/runs/14100376040/job/39495398374
Real set: https://github.com/man-group/ArcticDB/actions/runs/14100497273/job/39495728734

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...

• Have you updated the relevant docstrings, documentation and copyright notice?
• Is this contribution tested against all ArcticDB's features?
• Do all exceptions introduced raise appropriate error messages?
• Are API changes highlighted in the PR description?
• Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

[G-D-Petrov]

The StorageSetup class can easily be refactored to be more readable like this:

def aws_default_factory() -> BaseS3StorageFixtureFactory:
    return real_s3_from_environment_variables(shared_path=True)

def get_machine_id() -> str:
    """
    Returns machine id, or id specified through environments variable (for github)
    """
    return os.getenv("ARCTICDB_PERSISTENT_STORAGE_SHARED_PATH_PREFIX", socket.gethostname())


def create_prefix(storage_space: StorageSpace, add_to_prefix: str) -> str:
    def is_valid_string(s: str) -> bool:
        return bool(s and s.strip())

    mandatory_part = storage_space.value
    optional = add_to_prefix if is_valid_string(add_to_prefix) else ''
    return f"{mandatory_part}/{optional}" if optional else mandatory_part


def check_persistence_access(storage_space: StorageSpace, confirm_persistent_storage_need: bool = False):
    assert aws_default_factory(), "Environment variables not initialized (ARCTICDB_REAL_S3_ACCESS_KEY,ARCTICDB_REAL_S3_SECRET_KEY)"
    if storage_space == StorageSpace.PERSISTENT:
        assert confirm_persistent_storage_need, "Use of persistent store not confirmed!"


def get_arctic_uri(storage: Storage, storage_space: StorageSpace, add_to_prefix: str = None, confirm_persistent_storage_need: bool = False) -> str:
    check_persistence_access(storage_space, confirm_persistent_storage_need)
    prefix = create_prefix(storage_space, add_to_prefix)
    if storage == Storage.AMAZON:
        factory = aws_default_factory()
        factory.default_prefix = prefix
        return factory.create_fixture().arctic_uri
    elif storage == Storage.LMDB:
        return f"lmdb://{tempfile.gettempdir()}/benchmarks_{prefix}"
    else:
        raise Exception("Unsupported storage type:", storage)

[grusev]

I do not agree. There is a value of separating responsibility and working TestLibraryManager.

It provides better isolation and management. So I disagree with making those changes

[IvoDD]

I don't have a strong opinion whether these live inside a class or as separate functions. I think in this case the class only provides namespacing, which can also be done with a module but I don't think it matters hugely.

I think @G-D-Petrov 's suggestion provides a few simplifications which I find valueable:

• The main purpose of this being a class seems to be the caching of _aws_default_factory. I don't think creating it is expensive? Why bother caching it then? This way we need to worry about forgetting to initialize it the first time. I.e. we can just replace the __new__ with a aws_default_factory.
• The create_prefix is rewritten in a much more concise way.

To sum up, I don't mind leaving this a class for the namespacing but it would be nice to still simplify the logic we can

[grusev]

I have made suggested simplificiations in the class (part of them as google was added and other thing changed meanwhile). So code is simpler, but still in class where it should be. When there is module with lots of mixed functions classes can and should be used to provide namespacing and encapsulation and that is good style as it helps writing code - IDEs help, through intelli sense etc. Long files with lots of functions one scattered here another there mixed with other functions is actually not great example of code, and we are going to achieve that if we do not use classes. Classes provide clarity of purpose

[G-D-Petrov]

As we spoke yesterday, this LibraryManager is unnecessary because it duplicates a lot of the logic of Arctic's internal LibraryManager.
The only needed functionality is:

• having a function to create the persistent/modifiable Arctic client with the correct URIs
• having a function that constructs the correct names for the libraries
• some helper function for cleaning up modifiable libraries, which can just iterate over the libraries in the modifiable Arctic client

Every thing else here is can easily be handled though the arctic clients directly

[grusev]

Again - all tests work, pass and the abstraction isolates very well user from ASV etc. All points you make are perhaps valid but constitute totaly different approach. As I disagree with that approach on real grounds I cannot make those changes

TestLibraryManager isolates within itself all needs for user to understand the specifics of the structure and lets the person who writes the test to write any types of test using requested libraries.

It gives the creator full freedom to do what is needed to achive best result

It provides a way of work that eliminates the need of test author to know ASV internals and thus protects from problems that will arise during test execution

It also gives ability without change of test code to make changes in the structure of the the storage spaces.

All that is well tested and the framework itself is covered with tests that can be extended.

There is no point of making any changes rather than wasting more resources on this.

[grusev]

BTW, It is TestLibrary manager now

[grusev]

TestLibraryManager does not duplicate any work of Arctic Library manager. It provides very needed isolation for creating tests from where and how Arctic will create them. More info can be found in the new documentation of the class:

ArcticDB/python/arcticdb/util/environment_setup.py

Line 192 in f053c6a

This class is a thin wrapper around Arctic class. Its goal is to provide natural user

As a conclusion I do not argue that eventually a version without use of class and only functions can be created. I argue its value and need. There are many many arguments why not take this approach in the description there.One additional and very compelling is the fact that Arctic is class and in order to override class you need a class not functions. (Arctic is not base class for good reason)

I do find enough reasons that current implementation is better - one I can name is the fact that no one uses Arctic in man db to directly create libraries - there is and UI library manager. In all real implementation when you have certain specifics for management the infrastructure you override what is avail. Current implementation is exactly this. That is why it is ok andy change is rather waste of resources and eventual result is more likely to force implementation of similar thing at some point.

[G-D-Petrov]

This name is not very descriptive and the comments seems a bit misleading.
AFAICS this is a cache for the expected results thought the run.
The name/comment should reflect that.

[grusev]

Will make necessary changes

[G-D-Petrov]

should be renamed to something like index_start, then it doesn't need the comment

[grusev]

I think the naming is ok. It returns both start and the index frequence - hence the name get_index_info(), also the comment there suggests that:

def get_index_info(self):
    """
    Returns initial timestamp and index frequency
    """
    return (pd.Timestamp("2-2-1986"), 's')

[G-D-Petrov]

If it can be only 1 value, why do we even have it as a parameter?

[grusev]

The parameters of the tests are defined as such in each test case. Thus test case A parameters are memebers of class A not to the instance. Test case B parameters are for test case B. The function checks parameters of different classes

class AWSLargeAppendTests(AsvBase)
class AWS30kColsWideDFLargeAppendTests(AWSLargeAppendTests)

[grusev]

The test code in some tests do contain assertains about the certains ASV parameter values. Those assertions are needed because the tests can either not work if they do not have that specific value or they might work but produce false results.

that is additional thing added to the new implementation of tests. They do check 2 things:

• validity of the preconditions (usually ASV parameters, or setup)
• validity of test operations (see tests for batches for instance the asserts for batch operations, which are silent by default - ie they do not fail laoudly if error)

[G-D-Petrov]

num_sequential_dataframes += 1

[grusev]

but it is also correct in current form right?

[grusev]

fixed

[G-D-Petrov]

This function is a bit hard to follow, consider refactoring it to something like:

def initialize_update_dataframes(self, num_rows: int, num_cols: int, cached_results: LargeAppendDataModifyCache, 
                                 generator: SequentialDataframesGenerator):
    logger = self.get_logger()
    initial_timestamp, freq = self.get_index_info()
    timestamp_number = TimestampNumber.from_timestamp(initial_timestamp, freq)
    
    def log_time_range(update_type: str, df_key: int):
        time_range = generator.get_first_and_last_timestamp([cached_results[update_type][df_key]])
        logger.info(f"Time range {update_type.upper()} update {time_range}")

    def generate_and_log(update_type: str, num_rows: int, start_ts: pd.Timestamp):
        df = generator.df_generator.get_dataframe(number_rows=num_rows, number_columns=num_cols, start_timestamp=start_ts, freq=freq)
        cached_results[update_type][num_rows] = df
        log_time_range(update_type, num_rows)

    logger.info(f"Frame START-LAST Timestamps {timestamp_number} == {timestamp_number + num_rows}")

    # Full update
    generate_and_log('update_full_dict', num_rows, initial_timestamp)

    # Half update
    half = num_rows // 2
    timestamp_number.inc(half - 3)
    generate_and_log('update_half_dict', half, timestamp_number.to_timestamp())

    # Upsert update
    generate_and_log('update_upsert_dict', num_rows, timestamp_number.to_timestamp())

    # Single update
    timestamp_number.inc(half)
    generate_and_log('update_single_dict', 1, timestamp_number.to_timestamp())

    # Single append
    next_timestamp = generator.get_next_timestamp_number(cached_results.write_and_append_dict[num_rows], freq)
    generate_and_log('append_single_dict', 1, next_timestamp.to_timestamp())

[grusev]

makes sence!

[G-D-Petrov]

As we have discussed, this can be greatly simplified by decoupling the configuration for the population from the logic of executing the populating.
This can be done with a refactor like:

@dataclass
class LibraryPopulationConfig:
    """Immutable configuration for library population."""
    parameters: List[int]
    parameters_are_rows: bool = True
    fixed_rows: int = 1
    fixed_columns: int = 1
    symbol_prefix: str = ""
    use_auto_increment: bool = False
    with_metadata: bool = False
    versions_count: int = 1
    versions_mean: float = 1.0
    with_snapshots: bool = False

    def symbol_name(self, index: int) -> str:
        """Get the symbol name based on configuration."""
        prefix = f"symbol_{self.symbol_prefix}_" if self.symbol_prefix else "symbol_"
        return f"{prefix}{index}"
    
    def create_metadata(self) -> Dict[str, Any]:
        """Create metadata for symbols and snapshots."""
        if not self.with_metadata:
            return {}
        return DFGenerator.generate_random_dataframe(rows=3, cols=10).to_dict()


class LibraryPopulator:
    """
    Handles the actual population of a library based on a configuration.
    Separates the configuration from the execution.
    """
    def __init__(self, config: LibraryPopulationConfig, logger: logging.Logger, 
                 df_generator: DataFrameGenerator = None):
        self.config = config
        self.logger = logger
        self.df_generator = df_generator or VariableSizeDataframe()
    
    def populate(self, library):
        """Populate the library according to the configuration."""
        start_time = time.time()
        
        for i, param in enumerate(self.config.parameters):
            # Determine symbol index
            symbol_index = i if self.config.use_auto_increment else param
            symbol_name = self.config.symbol_name(symbol_index)
            
            # Determine rows and columns
            rows = param if self.config.parameters_are_rows else self.config.fixed_rows
            columns = self.config.fixed_columns if self.config.parameters_are_rows else param
            
            # Generate dataframe
            df = self.df_generator.generate_dataframe(rows, columns)
            
            # Create symbol
            symbol = library.create_symbol(symbol_name, df)
            
            # Add metadata if configured
            if self.config.with_metadata:
                symbol.set_metadata(self.config.create_metadata())
            
            # Create versions if configured
            if self.config.versions_count > 1:
                versions_list = self._generate_versions_list(len(self.config.parameters))
                for v in range(1, min(versions_list[i], self.config.versions_count) + 1):
                    version_df = self.df_generator.generate_dataframe(rows, columns)
                    version = symbol.create_version(version_df)
                    
                    # Add metadata if configured
                    if self.config.with_metadata:
                        version.set_metadata(self.config.create_metadata())
                    
                    # Create snapshot if configured
                    if self.config.with_snapshots:
                        snapshot = library.create_snapshot(f"snapshot_{symbol_name}_{v}")
                        if self.config.with_metadata:
                            snapshot.set_metadata(self.config.create_metadata())
        
        self.logger.info(f"Population completed in: {time.time() - start_time:.2f}s")
    
    def _generate_versions_list(self, number_symbols: int) -> List[np.int64]:
        """Generate a list of version counts for each symbol."""
        # Implementation would depend on your specific requirements
        # This is a placeholder based on the original code
        versions_list = np.random.poisson(self.config.versions_mean, number_symbols)
        versions_list = np.clip(versions_list, 1, self.config.versions_count)
        return versions_list.astype(np.int64)

The code is just a example that I got from a pass though Claude and can be simplified further e.g:

• there are parameter in the Policy that can be removed
• we probably don't need a LibraryPopulator class, as some helper functions that take a policy should suffice

[grusev]

As we discussed we also agreed that current code is not conflicting any recommendations. Additionally there is no real benefit of making that rewrite other than to pass Claude's suggestions. The code is just ok in the form that is now. No need to changed it

[grusev]

The current implementation is ok it goes for fluent pattern which is widely used in python and even in arcticdb (queries). The requested change would not add any additional value nor it will fix any error. Therefore it is not needed at this moment.

[IvoDD]

I don't have a strong opinion whether these live inside a class or as separate functions. I think in this case the class only provides namespacing, which can also be done with a module but I don't think it matters hugely.

I think @G-D-Petrov 's suggestion provides a few simplifications which I find valueable:

• The main purpose of this being a class seems to be the caching of _aws_default_factory. I don't think creating it is expensive? Why bother caching it then? This way we need to worry about forgetting to initialize it the first time. I.e. we can just replace the __new__ with a aws_default_factory.
• The create_prefix is rewritten in a much more concise way.

To sum up, I don't mind leaving this a class for the namespacing but it would be nice to still simplify the logic we can

[IvoDD]

fixture_cache is now unused?

[grusev]

done

[IvoDD]

Should all libraries have the same LibraryOptions?

Doesn't it make more sense to pass LibraryOptions to get_library? This way we can create two libraries with different options which might be useful for some tests.

[grusev]

get_library will return existing library in most of cases. Only if does not exist will create new library. Thus the library options is possible but not quite determined in the case when library exists what to do - to check if lib is having same options or skip and disregard. Still confusing.

The hole TestLibraryManager is thin wrapper over Arctic, which goal is to "hide" details of internal management associated with how exactly placement of libraries happen and where and how to exactly locate and remove them. The argument that we can live without classes is not good as if we could we would have not had fixtures as classes at first place. And exactly this is this class. It provides extension to functionality of Arcitc class which is suited to do jobs better in very specific environment.

Thus a possible and better options in this case are:

• define create_library method ... now or when we actually need it.

• workarround with what we have currently:
  tlm.library_options = LibraryOptions(...)
  lib = lib.get_library(...)

First proposal is clean why, second is workarround. But yet we do not have scenario that requires that, hence create_library was not implemented.

That is something that specialized class provides over free floating functions - encapsulation, emergent design (extend when you need, change easily something when new info arrises without breaking tests etc)

Classes are the natural way to grow in complexity and need, as they require use of the whole contract not only parts of it function by function, which in time would errode the overall specialization and will lead to conflicts of requirements.

[IvoDD]

Isn't this storage_type not lib_type?

[grusev]

fixed

[grusev]

The for methods serve very different purposes. Their usage scenarios is now documented as their use might not be that obvious

[IvoDD]

Why not just the simpler lib_name.startswith(f"{LibraryType.Modifiable}_{self.name_benchmark}_{os.getpid()}_")?

[IvoDD]

All these clear functions can use a common clear_modifiable_with_prefix(self, prefix)
And clear_all_modifiable_from_this_process can pass the prefix with the pid wheras clear_all_benchmark_libs can pass the prefix without the pid.

[IvoDD]

Also remove_all_modifiable and remove_all_test_libs can also use the private prefix method.

[IvoDD]

This doc is a leftover from the refactor proposal. We can probably just remove it, from the name it is clear enough what this does.

[IvoDD]

I added quite a few small changes suggestions.

A few more imporant things to discuss maybe offline:

• random seeding
• list symbol tests with respect to symbol list compaction

[IvoDD]

Also remove_all_modifiable and remove_all_test_libs can also use the private prefix method.

[IvoDD]

I find the notion of parameters which can be either rows or columns a little confusing.
Why not just use num_rows: Union[int, List[int]] and num_cols: Union[int, List[int]]?
This way it's a bit clearer, if one of them is just an int it applies to all symbols if it is a list it ised similarly to the parameters. If they are both lists we should assert they have the same size.

[IvoDD]

This will also need including a num_symbols. In general if the constructor receives the num_symbols, num_rows, and num_cols and asserts that if rows or cols is a list it has the same length as num_symbols I think that would be quite easy to use

[IvoDD]

Anyway then the symbol naming becomes a bit less clear. Up to you

[IvoDD]

Nit: I find the names starting with generate_ a little confusing. Sounds like they themselves will genereate something. Why not just call them set_with_snapshots etc.

[grusev]

will think of better name

[IvoDD]

better use for index, param_value in enumerate(self.parameters)

[IvoDD]

Maybe it makes more sense to generate a new dataframe for each version? Instead of each version having the same dataframe. This will also be useful if we decide to allow (via a LibraryPopulationPolicy flag) the ability to update instead of write

[grusev]

For the scenarios that are currently implemented we do not need new dataframe on each write. That is the reason this was not implemented. And yes there is value of having new dataframes on each write. But that is expensive operation and should be approached carefully.

Still there is already a class that covers the scenario with new dataframes - SequentialDataframesGenerator, it is used in append scenarios.

Once a real case with Library Populator arises an optoion can be easily added for a new DF on each new write.

[IvoDD]

Name should be LIST_VERSIONS to not collide with the above

[grusev]

correct!

[IvoDD]

Unused

[IvoDD]

pid is already in the library, fine to use a more generic name

[grusev]

yep we may choose to use or not :-) In our case it really does not matter what the name is

[IvoDD]

Remove comment?

[IvoDD]

That's why I wanted to move this function in utils. A very identiacal function was used in read_batch_functions.py iirc

[grusev]

will do that

[IvoDD]

Here we are passing the same seed to all random_string calls? That's the behaviour I specifically wanted to avoid

[grusev]

Indeed - quick replacement without thinking that this is special case. Thanks for catching!

[IvoDD]

Maybe remove the self.__seed since we no longer need to pass it around. We can just do the if below.

[IvoDD]

Doc is not up to date

[poodlewars]

Why do we need to delete the old library? What would happen if we just left it lying around? If these steps are necessary we should have some quick docs for people telling them how to do this - I would have to reverse engineer how to do this cleanup

[grusev]

Work with shared persistence storage requires indeed a handbook for what to do and what not to do. And I will create a wiki page for that.

Here is what is quick explanation why this is need for that test.

When a test create a library on the shared persistent space, that library is considered to stay. Consequent test executions check that this library is there and continue the test or create it once again. Library structure is not checked (previous version contained structural checks of library and not just having checked its name, which was considered as overly complicating thing)

Now if the parameters of the test change, that would automatically trigger different code paths in test, but since the library was created once with old parameters the check will pass and it will be considered created, thus tests done on the library with those new params will produce either misleading results, or they will simply do not work. Both outcomes are possible.

Therefore there are 2 ways to battle this problem:

• having comments like this to assure whoever is changing parameters does clean the previous library first.
• having a more complicated name where library has the parameters encoded as suffix. Then a change in parameters will simply mean a new library created on the persistent store to stay. The end effect could be many libraries created and persistent storage bloated, which actually defeats its purpose. (For that there is already test storage space which should be used)

---

In other words persistent storage comes with logistics complications and therefore management should be applied. That is counterintuitive to most developers as usually tests are considered isolated from each other, and thus errors are possible. But in fact tests can and should be created considering also such persistence factor. Still they require some additional management

[grusev]

Docs will be prepared, for now the comment is changed

[IvoDD]

Should be LibraryType.PERSISTENT.value

[IvoDD]

I think this comment is confusing. Are you suggesting to commit a change to delete the library inside the github runners. That seems like an odd process, because then we would have to revert it.

I think it would be better if instructions describe how to do it locally instead of on github runner.
What do you think about the following:

If you plan to make changes to parameters, consider that library already may exist created with different number
of rows. Therefore, you need to either:
- Rename the library by changing `name_benchmark` or by adding a `lib_suffix`. Note that this will keep the old library around.
- Clean up the old library with (which needs to be run with the same environment variables as the github runner):
       library_manager = TestLibraryManager(storage=Storage.AMAZON, name_benchmark="READ_WRITE")           
       library_manager.remove_all_persistent_libs_for_this_test()

[grusev]

actually the process is much simpler and will leave it to the wiki which I currently work on: https://github.com/man-group/ArcticDB/wiki/ASV-Benchmarks:-Real-storage-tests

as we are talking about persistent tests which are accessed by all no specific vars are needed just simple 2-3 step process which will be explained there and the comment will have a link to it

[poodlewars]

Please update the commit message to something meaningful when you merge this