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

Remove unsafe colorama.init #1738

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Remove unsafe colorama.init #1738

merged 1 commit into from
Mar 2, 2023

Conversation

infwinston
Copy link
Member

@infwinston infwinston commented Mar 1, 2023

Fixes #1694.
After removing the init(), I was trying to add

from colorama import just_fix_windows_console
just_fix_windows_console()

as suggested by https://github.com/tartley/colorama#initialisation

But this function only introduced after 0.4.5 and we pin colorama<0.4.5 because it conflicts with aws-cli for some reason.

'colorama<0.4.5',

Related issue: #1323
Is this still the case after we upgrade ray version? @Michaelvll

or is it safe to add colorama.init() in sky/__init__.py?

Tested (run the relevant ones):

  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@infwinston
Copy link
Member Author

@romilbhardwaj Could you please help testing if with this branch, whether the color will show up on windows?

@Michaelvll
Copy link
Collaborator

Thank you for submitting the PR @infwinston! The older version of awscli requires a colorama before 0.4.5. Can we try to install a later awscli and launch an aws VM to see if the problem still exists?

@infwinston
Copy link
Member Author

infwinston commented Mar 2, 2023

Thanks @Michaelvll . looks like aws-cli also pins the colorama version before 0.4.5..
https://github.com/aws/aws-cli/blob/develop/setup.py#L31

I launched a windows VM to test what's going on without init()
I can reproduce the color issue. however, it seems to me that after import sky the issue has gone.
I think that's because some of the other packages we import already run colorama.init() so we don't need to call it again.
does it make sense to you?
image

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation @infwinston! The changes look good to me.

@infwinston infwinston merged commit 3635b0d into master Mar 2, 2023
@infwinston infwinston deleted the fix-spot-colorama branch March 2, 2023 22:04
sumanthgenz pushed a commit to sumanthgenz/skypilot that referenced this pull request Mar 15, 2023
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.

[Spot] FAILED_CONTROLLER error with spot launch after 30hrs of retry
2 participants