Feature Request: Wrap converters in unified context and results object to simplify usage and testing

[neilb-sq]

Summary

At the moment, each converter takes a number of boilerplate parameters to do with authentication and upload that aren't necessarily needed to perform a conversion. I propose that instead of this, the converters only take the parameters needed to convert to the intermediary objects and/or the final schema objects, and the uploading to Evo can be a separate process with its own parameters.

Background

Right now the converters look like:

def convert_yourfiletype(
    filepath: str,
    epsg_code: int,
    evo_workspace_metadata: Optional[EvoWorkspaceMetadata] = None,
    service_manager_widget: Optional["ServiceManagerWidget"] = None,
    upload_path: str = "",
) -> list[ObjectMetadata]:

In some cases (but not all), setting upload_path to None will cause the geoscience objects to be returned instead of uploading them. If this is the case, the evo_workspace_metadata/service_manager_widget are not required, and the return format of the function changes. This makes static analysis for type checking more complex.

A consistent return format that is a wrapper for the conversion result would allow for a consistent upload interface, and make features like versioning only happen in one place.

This might look like:

def convert_yourfiletype(
    filepath: str,
    epsg_code: int
) -> ConversionResult:

class ConversionResult:
  geoscience_objects: ObjectCollector

  ...
  def upload(self, upload_path: str, overwrite: bool = False) -> list[ObjectMetadata]:
     ...

Also, I think the authentication/configuration piece could be handled with a context manager:

with EvoContext(evo_workspace_metadata=meta):
  convert_yourfiletype(filepath="/path/to/my.file")
    .upload("my-upload/path")

This would allow for other optional configuration around the interface to Evo to be kept in context, rather than passed through all the converters. Currently, much of this kind of context is already wrapped in EvoWorkspaceMetadata/ServiceManagerWidget, but there could be additional context in future.

User Story

As a user of this library
I want to have a consistent, lean, chainable API for converting different file types
So that my code is cleaner and easier to read

Acceptance Criteria

• All converter functions have the same parameters that work the same way
• Uploading is an optional step for all converters
• Easy to test the converter functions without any Evo context
• Tests updated to work
• Readme updated with new expectations

Out of Scope

• Implementing overwrite/revisioning as part of this, as this is happening elsewhere
• Changing the way files are loaded for conversion, as the underlying converter libraries typically want to handle this themselves

References

N/A

Implementation Notes

Definition of Done

• All acceptance criteria met
• Code reviewed and approved by maintainers
• Documentation complete and reviewed
• Tests passing in CI/CD pipeline

[daniel-kinney-seequent]

Establishing an idiom for separating convert from upload does make sense. 👍

Having an Evo context makes sense. 👍

I'm not sure about making the context global, because I can imagine wanting to run uploads in parallel, possibly with different Evo contexts.

[neilb-sq]

A context manager isn't "global" except to the things inside it. At the moment given publish_geoscience_objects is a synchronous function you can't run uploads in parallel anyway, though in the future it could be made async and use multiple async context managers to do multiple uploads at once. I imagine in reality this code will run in the function compute service such that each conversion has its own context anyway.

I was just reaching for a way to keep the configuration stuff out of the way so the functions don't have to pass it around and can reference a ContextVar as required. It's especially pertinent in local testing/Jupyter where it would be nice to push some context and plug away making calls without having to think about it.

Happy to refine the approach.

[ArthurGW]

I like the idea of this structure in the current approach.

However, it does come with a downside - it makes it harder to change to the approach where objects are uploaded as soon as they are created, rather than in a batch at the end. That approach might save on disk usage, and might be more performant as more is going on in parallel. In the back of my mind that was where we might go in future, but there's no actual plan for that.