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

Update CI and switch to python 3.11 #57

Merged
merged 6 commits into from
Mar 14, 2024
Merged

Update CI and switch to python 3.11 #57

merged 6 commits into from
Mar 14, 2024

Conversation

danielhfrank
Copy link
Contributor

@danielhfrank danielhfrank commented Mar 13, 2024

Background

Description

  • Updates setup-gcloud workflow in CI to current version (easy)
  • Business logic test failures were due to behavior changes in fsspec that affected the glob function, see Make glob consistent with glob.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 behavior
  • Additionally, began running tests on 3.11, and some tests broke on behavior around an StrEnum alternative that we were using. Elected to switch to just test on 3.11 (per slack), and bring in the stdlib StrEnum

Discussion

  • The fsspec changes crept into scope because we don't pin our dependencies, but rather specify minimum versions. Thus it was possible to pull in the breaking changes automatically, and since we don't develop in this repo often, not notice. We might consider either pinning dependencies, or running tests on main periodically. I assume that we don't pin dependencies in order to maintain this codebase as a "library" that other codebases can pull in and have control over their own deps. In those circumstances, we might consider scheduling a weekly CI run or something

cc @ravwojdyla

@danielhfrank danielhfrank changed the title Update CI and prepare for python 3.11 Update CI and switch to python 3.11 Mar 13, 2024
@danielhfrank danielhfrank marked this pull request as ready for review March 13, 2024 21:32
Copy link
Member

@dhimmel dhimmel left a 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.

@dhimmel
Copy link
Member

dhimmel commented Mar 13, 2024

We might consider either pinning dependencies, or running tests on main periodically. I assume that we don't pin dependencies in order to maintain this codebase as a "library" that other codebases can pull in and have control over their own deps. In those circumstances, we might consider scheduling a weekly CI run or something

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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 😉

@danielhfrank
Copy link
Contributor Author

@danielhfrank danielhfrank merged commit 5375ce4 into main Mar 14, 2024
1 check passed
@dhimmel dhimmel deleted the df-update-ci branch March 14, 2024 19:39
dhimmel added a commit that referenced this pull request Mar 14, 2024
dhimmel added a commit that referenced this pull request Mar 15, 2024
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.

3 participants