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

Add .assign_temporary_storage method via TemporaryStorageMixin #286

Closed

Conversation

cisaacstern
Copy link
Member

@cisaacstern cisaacstern commented Feb 15, 2022

A lightweight (albeit possibly partial) solution for #276 which is hopefully simple enough to merge (and release) before OSM2022, to aid @rwegener2's ongoing work on #281.

To achieve the same result as represented by the code block in #276 (comment), users would now need only call:

recipe.assign_temporary_storage()

I considered setting true defaults (as opposed using the method call), but ultimately thought requiring users to make this method call had some instructive value, insofar as it conveys the notion that storage targets for a given recipe are customizable.

@rabernat, by way of background, Rachel and I believe we need some type of simplification for OSM in this area. I am by no means attached to this particular solution, it simply represents my best effort at something simple enough to merge (and then document!) on our short runway. Totally open to any alternative if you can think of a better one.

@rabernat
Copy link
Contributor

I agree this is needed. But I have some different ideas about how to implement it.

I would like to first create a StorageConfig object that wrap target, cache, and metadata targets. So when we "assign storage" to a recipe, we just have one parameter to set. This will be an API change that will break many existing tutorials and examples.

Then we will make a default factory for a TempStorageConfig. This will effectively allow us to just remove the storage configuration from all the examples.

I would like to work on this (along with #279) early next week once I get out from under this week's deadlines. Would that timeline work for you? So for the purposes of planning the tutorial, you can definitely assume that this will be resolved soon.

@cisaacstern
Copy link
Member Author

Implementation timeline sounds good. Rachel and l are meeting to continue tutorial prep on Thursday, at which point it would be useful to have some (even placeholder) idea of what your API redesign will look like, so we can drop that into our evolving draft.

Sounds like rather than

recipe.assign_temporary_storage()

you're envisioning a one-liner like

recipe.storage_config = TempStorageConfig()

?

@cisaacstern
Copy link
Member Author

Then we will make a default factory for a TempStorageConfig. This will effectively allow us to just remove the storage configuration from all the examples.

Oh... IIUC, this then becomes a true default on the recipe classes, insofar as users can just jump right into execution, without even a one-liner. That works! Closing this now.

@rabernat
Copy link
Contributor

It would probably be a class like this:

@dataclass
class StorageConfig:
    target: FSSpecTarget
    cache: Optional[FSSpecTarget] = None
    metadata: Optional[MetadataTarget] = None

A function like this

def temporary_storage_config():
    # create temporary directories
    return StorageConfig(...)

and a mixin like this

@dataclass
class StorageMixin:
    storage_config: StorageConfig = field(default_factory=temporary_storage_config)

This would be mixed into both recipe classes (as yours is now). The default factory would automatically create a new temporary storage config on every recipe. The main API difference with what you have here would be that temporary storage would be the default, with no explicit .assign_temporary_storage() step .

If you would like to implement this yourself, please go right ahead!

Sorry for being so specific here. I have spent quite some time thinking this through and have a strong opinion on how it should be done.

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.

2 participants