-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update CI and switch to python 3.11 #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's give @ravwojdyla some time to review since he'll be most familiar with whether the small number of functional changes could have unintended consequences.
I'm in favor of unpinned dependencies with scheduled CI so we're aware of whether anything breaks due to upstream changes. |
@@ -89,7 +89,9 @@ def download_artifact(artifact: FSArtifact, local_dir: PathType) -> str: | |||
prefix = artifact.main_dir | |||
# Note: glob results don't have fs scheme | |||
prefix_no_scheme = re.sub(FSArtifact._fs_scheme_regex, "", prefix) | |||
to_copy = src_fs.glob(artifact.files_pattern) | |||
# Root directory can be included in glob results if a trailing ** is used (which it often is) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I remember this being inconsistent across FSs, and I remember disliking this logic a lot, is there any chance to simplify the current implementation now given the (assumption!) more consistent glob results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm, I gave it another read-through, and I don't see too much wrong with it. If anything, I guess I could see using the more consistent glob behavior to change what glob patterns were used upstream, but that sounds like potentially a complicated effort to track down callsites.. and moreover, I don't think it would rank high on the list of company objectives 😉
|
Background
Description
fsspec
that affected theglob
function, see Makeglob
consistent withglob.glob
fsspec/filesystem_spec#1382 and Better double asterisks**
support fsspec/filesystem_spec#1329 . I changed one test whose expectations were no longer valid, and otherwise changed a bit of application logic to match the pre-existing behaviorStrEnum
alternative that we were using. Elected to switch to just test on 3.11 (per slack), and bring in the stdlibStrEnum
Discussion
cc @ravwojdyla