-
Notifications
You must be signed in to change notification settings - Fork 4.1k
chore: Upgrade Python requirements #37153
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
Conversation
List of packages in the PR without any issue.
|
These Packages need manual review..
|
lxml[html-clean,html_clean]==5.3.2 | ||
lxml[html-clean]==5.3.2 |
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.
@feanil: Does the install work for you? On a sandbox or somewhere? I'm not sure what changed that caused this change, but it is causing our pipeline to break with:
WARNING: lxml 5.3.2 does not provide the extra 'html_clean'
...
AssertionError: Internal issue: Candidate is not for this requirement lxml[html-clean,html-clean] vs lxml[html-clean]
Not sure if this is a 2U issue, but I'm guessing not. Not sure how to correct this. I'm trying some things, but if it doesn't work for you, it might be good to revert first.
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.
- I just realized that
pip-tools.txt
changed. Maybe that is causing the issues? - I created a revert PR here: Revert "chore: Upgrade Python requirements" #37173
- I don't have time to get to the bottom of this at this very moment.
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.
[inform] I constrained pip-tools, rather than reverting:
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.
@feanil: I'm still not sure:
- Why this didn't break until our pipeline, and whether it would have broken for a sandbox or in Tutor dev, etc.
- This took me several hours of work, and wondering if I should have just reverted. However, reverting would have just been a temporary fix because any
make upgrade
would have broken things again.
- I know once 2U is deploying off named releases, this would no longer affect us. But when and where would it affect the community, and is there a process I could kick off sooner so this doesn't fall in 2U while still deploying off master?
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.
1--
I'm also surprised this didn't break until your pipeline. I thought that some unit tests installed edx-sandbox
requirements, I guess not?
2--
Sorry to hear you spent so much time figuring out whether to revert. IMO you were justified to revert as soon as you felt this way:
Not sure if this is a 2U issue, but I'm guessing not. Not sure how to correct this.
The idea is that we want 2U engineers to make a good faith judgement call that the PR is a general problem before reverting, rather than just reverting anything at all that broke on edx.org. Given your edx-platform experience level, I trust you to make the call to revert even if it's just "based on this error log and my understanding of how pip works, this is very likely not a 2U problem". I tried to capture that on this page but if you think it could be clearer, edits are welcome.
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.
Thanks @kdmccormick. I wasn't actually clear what was going wrong and why it didn't break until our pipeline, and feared we might be caching something strange.
In any case, maybe getting a simple reminder that I can still free to revert under these types of situations. That still would mean that one of us would have to get to the bottom of this, but we'd have an extra couple of days.
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.
@robrap sorry about that, this lxml[html-clean]
vs lxml[html_clean]
issue has been the bane of upgrade management. If some of your added dependencies are not sufficiently up-to date with their dependency on lxml[html-clean]
I've seen this happen. When you have a sec if you could post a more full stack-trace and ideally the command you ran that produced the error that would be helpful in hopefully re-producing this before this sort of thing merges.
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.
@feanil: pip-tools thinks the problem lies with us. I added more details to the remove constraint issue:
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Python requirements update. Please review the changelogs for the upgraded packages.