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 pyyaml to 6.0.1 #596

Merged
merged 1 commit into from
Aug 15, 2023
Merged

update pyyaml to 6.0.1 #596

merged 1 commit into from
Aug 15, 2023

Conversation

orcutt989
Copy link
Contributor

@orcutt989 orcutt989 commented Jul 25, 2023

Fixes broken pyyaml arm64 build in our CI.

@orcutt989 orcutt989 marked this pull request as ready for review July 25, 2023 17:34
Copy link
Collaborator

@nicolasochem nicolasochem left a comment

Choose a reason for hiding this comment

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

thanks

Copy link
Collaborator

@harryttd harryttd left a comment

Choose a reason for hiding this comment

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

i'm not sure this should be merged yet. It is a major version update of a package which seems to be a dep of another. If the code hasn't been tested we don't know what will happen.

@harryttd
Copy link
Collaborator

Please see my comment here: #595 (comment)
I believe we should wait for the maintainers to fix.

@orcutt989
Copy link
Contributor Author

orcutt989 commented Jul 25, 2023

Please see my comment here: #595 (comment) I believe we should wait for the maintainers to fix.

Thanks @harryttd. Yes I looked at the PR and I wasn't able to understand the issue. It seems as if the arm64 build randomly stopped working for PyYAML 5.4. Could you elaborate on how the PR could be related?

This seems more relevant possibly yaml/pyyaml#724

I did some research and along with the above- the consensus seems to be to upgrade to 6.0.1.
I spoke with @nicolasochem about upgrading and he didnt think there would be issue.

Happy to wait, but we have a broken pipeline at the moment.

@orcutt989
Copy link
Contributor Author

I see now how the Cython PR is relevant. However I dont think its going to be fixed.

yaml/pyyaml#734 (comment)

@harryttd
Copy link
Collaborator

Yeah.. as the days go on they don't seem to want to fix this for older versions yaml/pyyaml#731

Did you test the code to make sure it still works with the major release version?

@nicolasochem
Copy link
Collaborator

Did you test the code to make sure it still works with the major release version?

We are not doing anything fancy with pyyaml so I'll assume it can still parse our yaml fine.

@nicolasochem nicolasochem merged commit f5f784c into master Aug 15, 2023
20 checks passed
@orcutt989 orcutt989 deleted the update-pyyaml branch September 21, 2023 19:15
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