Fix for glob patterns in artifacts #1567
Conversation
| if "*" in str(artefact): | ||
| raise ValueError("Glob patterns not supported for Snowpark artifacts.") | ||
| return artifacts | ||
| def get_artifacts(self) -> List[PathMapping]: |
There was a problem hiding this comment.
NADE's path globbing logic is centralized in BundleMap. Any particular reason we can't reuse it here instead of doing a one-off implementation? I'm worried that different glob expansion implementations will end up making things confusing for customers since the specific globbing semantics can diverge over time (and reading what's below, might have diverged already TBH).
There was a problem hiding this comment.
Should we call BundleMap from Streamlit and Snowpark plugins, or extract this code to any common module?
There was a problem hiding this comment.
If BundleMap is generic and easy to use we can use this implementation. If it's too tightly coupled with NA implementation I'm in favor of solving this in separate PR.
There was a problem hiding this comment.
It's intended to be generic, but feel free to take a look and tell me what you think.
| main_file: streamlit_app.py | ||
| artifacts: | ||
| - streamlit_app.py | ||
| - pages/*.* |
There was a problem hiding this comment.
Out of curiosity, and particular reason to use *.*, instead of just *? I've never seen that pattern used before.
There was a problem hiding this comment.
No particular reason. In this context they should have the same result.
| _artifacts = [] | ||
| for artifact in self.artifacts: | ||
| if "*" in str(artifact): | ||
| root = artifact.parent.absolute() if "**" not in str(artifact) else Path(".") |
There was a problem hiding this comment.
So, is this also supposed to support **/*-style paths? That would be excellent!
There was a problem hiding this comment.
Yes, it should
sfc-gh-jsikorski
force-pushed
the
jsikorski/create_getter_for_artifacts
branch
from
d13c5f6
to
01f7b02
Compare
Co-authored-by: Zachary Blackwood <zachary.blackwood@snowflake.com>
sfc-gh-turbaszek
force-pushed
the
jsikorski/create_getter_for_artifacts
branch
from
547af2a
to
5f5aef2
Compare
Pull request was closed
Pre-review checklist
Changes description
Fix for #1529 - allows using glob patterns in artifact fields.
Adds getters, to ensure that the Snowpark artefacts are returned as a list of PathMappings