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

chore: popen using utf8 encoding #742

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

fatelei
Copy link
Contributor

@fatelei fatelei commented Nov 9, 2023

Describe the changes you have made:

Reference any relevant issue (Fixes #000)

fix #209 using utf8 encoding instead of cp1255

  • I have performed a self-review of my code:

I have tested the code on the following OS:

  • Windows
  • MacOS
  • Linux

AI Language Model (if applicable)

  • GPT4
  • GPT3
  • Llama 7B
  • Llama 13B
  • Llama 34B
  • Huggingface model (Please specify which one)

@fatelei
Copy link
Contributor Author

fatelei commented Nov 12, 2023

if we want to customize env not affect system env, copy it may be better

@ericrallen
Copy link
Collaborator

@fatelei You’re right. I deleted my last review question after looking at things a bit more closely.

Going to test this one out in a bit so we can get it merged.

Thanks for the contribution!

Copy link
Collaborator

@ericrallen ericrallen left a comment

Choose a reason for hiding this comment

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

Based on my limited testing, this seems to work as expected.

I've never encountered the error that other folks were getting, though.

@ericrallen ericrallen added the Triaged Issue has been reviewed by maintainer label Nov 13, 2023
@fatelei
Copy link
Contributor Author

fatelei commented Nov 14, 2023

Based on my limited testing, this seems to work as expected.

I've never encountered the error that other folks were getting, though.

what about unit tests

@ericrallen
Copy link
Collaborator

@fatelei If you can think of a good way to recreate a situation that triggers the error and then add a unit test where we make sure that interpreter can still execute code, that would be great.

I haven't had a chance to put brainpower towards it because I'm not sure what the underlying environment setting that causes an issue in the first place is.

I guess we could just set PYTHONIOENCODING to something else in the test, but then we'd just be testing if setting the encoding works rather than if setting it and passing the environment to the subprocess fixes the underlying issue with the system having a different encoding.

@KillianLucas
Copy link
Collaborator

This seems like a good idea— utf-8 handles diverse output chars including the European chars the issues seemed to be having difficulty with. Pushing to pip soon so I'm going to merge this.

I think in the future a good unit test would try out a bunch of different non-English chars, but something tells me that might only be useful once we can be testing things across a bunch of different OS/terminals, not just the github ubuntu non-GUI one.

Great work @fatelei and thanks @ericrallen for reviewing!

@KillianLucas KillianLucas merged commit 3f0dbdb into OpenInterpreter:main Nov 15, 2023
1 check failed
joshuavial pushed a commit to joshuavial/open-interpreter that referenced this pull request Nov 16, 2023
chore: popen using utf8 encoding
Former-commit-id: 3f0dbdb
Former-commit-id: 855b01217d5c87955a15bfdd2c66851f3c773151
Former-commit-id: a59fd5d0eea3bacaee81feb82bdf9b5252250a40 [formerly 2810a9ceca33701f274bbd8b7aaa3d4652cdfaf4]
Former-commit-id: 558a4f501d5fd670a1ac58ac8bb55bb4fc579682
joshuavial pushed a commit to joshuavial/open-interpreter that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged Issue has been reviewed by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnicodeDecodeError - help will be appriciate!
3 participants