Skip to content

Conversation

@jpn--
Copy link
Member

@jpn-- jpn-- commented Feb 7, 2023

Initial implementation for #645

@jpn-- jpn-- requested a review from dhensle February 7, 2023 18:01
Copy link
Contributor

@dhensle dhensle left a comment

Choose a reason for hiding this comment

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

Generally looks good and seems to work as designed for me. Wondering if the default pipeline_file_name setting should change in config.py to remove the ".h5". (Had to do some digging here for the actual setting to get this feature turned on!). Note that I think changing the default setting will also use this parquet functionality in the pipeline CI testing, which I think is desirable. Based on the getting_started.ipynb notebooks, I think the .h5 version should be specified explicitly in the settings for the prototype_mtc example.

If the pipeline_file_name setting ends in ".h5", then the pandas
HDFStore file format is used, otherwise pipeline files are stored
as parquet files organized in regular file system directories.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume documentation on this setting will to be addressed in the other Pydantic task?

although when using the parquet storage format this file is stored as "None.parquet"
to maintain a simple consistent file directory structure.
If the
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like unfinished thought here...

store.joinpath(table_name).mkdir(parents=True, exist_ok=True)
df.to_parquet(store.joinpath(table_name, f"{checkpoint_name}.parquet"))
else:
complib = config.setting("pipeline_complib", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another setting to not get lost in the Pydantic task.

@jpn--
Copy link
Member Author

jpn-- commented Nov 22, 2023

This PR was superseded by #654

@jpn-- jpn-- closed this Nov 22, 2023
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