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

Poetry scripts are breaking the documented behavior of sys.argv #7528

Closed
4 tasks done
delucca opened this issue Feb 17, 2023 · 32 comments
Closed
4 tasks done

Poetry scripts are breaking the documented behavior of sys.argv #7528

delucca opened this issue Feb 17, 2023 · 32 comments
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged

Comments

@delucca
Copy link

delucca commented Feb 17, 2023

  • Poetry version: 1.3.2

  • Python version: 3.10

  • OS version and name: Arch Linux

  • pyproject.toml: Not relevant (more info on the issue)

  • I am on the latest stable Poetry version, installed using a recommended method.

  • I have searched the issues of this repo and believe that this is not a duplicate.

  • I have consulted the FAQ and blog for any relevant entries or release notes.

  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option) and have included the output below.

Issue

This issue is a follow-up of flask#2588. You can get the full context there.

When running a script, Poetry is mutating the sys.argv. This breaks and violates the documented behavior of sys.argv and, therefore, must be avoided.

Regarding the practical issue (besides breaking the documented behavior of sys.argv), Flask reloader relies on sys.argv to reload the application automatically. Due to this, we can't launch a Flask application with the reloader enabled due to this issue.

You can replicate this bug by doing the following:

  • Create a new Poetry application
  • Install Flask on it
  • Setup a minimal Flask application
  • Add a poetry script that will launch that application, something like:
[tool.poetry.scripts]
dev = 'my_package.main:run'
  • On your shell, set FLASK_DEBUG=1
  • Execute the Poetry script: poetry run dev
  • You're going to get an error, saying that Python wasn't able to open the file called dev (due to the fact that Poetry script is setting sys.argv = ["dev"]
  • Now, set FLASK_DEBUG=0
  • Launch the script again
  • You'll see that, now, it works as expected

I've already opened that issue on Flask, and you can check Flask's team explanation about why this issue isn't related to them on this comment

@delucca delucca added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Feb 17, 2023
@delucca delucca changed the title Poetry scripts are breaking the document behavior of sys.argv Poetry scripts are breaking the documented behavior of sys.argv Feb 17, 2023
@finswimmer
Copy link
Member

Hey,

sounds like a duplicate of #965 which is already resolved by #6737. This will be included in the 1.4 release.

fin swimmer

@davidism
Copy link

davidism commented Feb 17, 2023

I'm not sure if I'm interpreting the docs/patch correctly, but that only appears to be fixing the issue for installed scripts. Does Poetry install scripts in editable mode? When is a script installed vs not?

sys.argv[0] should be the absolute path of the module being imported. Python itself does this with python hello.py and python -m hello, and other tools expect this.

@radoering
Copy link
Member

If I remember correctly:

If you don't run poetry install or run poetry install --no-root, the root project as well as its scripts are not installed. Then, sys.argv[0] will still only contain the name of the script and not the absolute path.

If you run poetry install, the root project is installed in editable mode and its scripts will be installed in the target environment. Then, sys.argv[0] will be the absolute path of the installed script.

@davidism
Copy link

davidism commented Feb 17, 2023

Yeah, so in that case the linked PR won't help, since while you're developing you'll have the project installed in editable mode. Poetry currently turns poetry run dev into

/home/david/.cache/pypoetry/virtualenvs/hello-y1FJtfO2-py3.10/bin/python -c "import sys; from importlib import import_module; sys.argv = ['dev']; sys.exit(import_module('hello').run())"

import sys
from importlib import import_module
sys.argv = ['dev']
sys.exit(import_module('hello').run())

but it needs to generate a slightly different command that will set sys.argv to the imported module's filename, something like

import sys
from importlib import import_module
mod = import_module('hello')
sys.argv[0] = mod.__file__
sys.exit(mod.run())

@radoering
Copy link
Member

while you're developing you'll have the project installed in editable mode

I'm saying that with the PR sys.argv[0] will be absolute if the project is installed (even in editable mode). As far as I know, poetry does always install the root project in editable mode.

@davidism
Copy link

even in editable mode

Ah ok, that makes more sense.

@wagnerluis1982
Copy link
Contributor

There are +/- 2 sides here, I think:

  1. Poetry should be consistent in the behaviors of poetry run <script> and poetry shell, <script>. With Fix incorrect sys.argv[0] path when calling project scripts #6737, this was resolved.
  2. sys.argv must be always self-callable (i.e. follow documented behavior), which in that case Poetry only guarantee with an installed script.

As a side note, I think we can achieve both with Poetry by not rewriting sys.argv on uninstalled scripts.

@wagnerluis1982
Copy link
Contributor

@davidism by checking documented behavior, I don't understand how you intend to rely on it.

After a few random checks, I got that python -c don't get what you wish.

For instance, I expected the following to print ['-c', 'import sys; print(sys.argv)'] in order to be self-callable, but didn't.

$ python3.10 -c "import sys; print(sys.argv)"
['-c']

@davidism
Copy link

davidism commented Feb 19, 2023

This issue was noticed because of Werkzeug's reloader, but it's not actually related to fixing that issue or being able to relaunch a process. The issue with Poetry is that it's changing sys.argv[0] to something unexpected.

The sys.argv[0] = "-c" matches Python's documented behavior for the python -c code invocation, so that's also an acceptable fix, although it wouldn't provide launch details.

Fixing Poetry by making it use sys.argv[0] = "full path to module" would be best. That matches the behavior of python -m module, which Poetry is basically emulating with its entry point.


You proposed one solution, which is to not rewrite sys.argv. I think that's fine. You could also fix it by always rewriting it to imported_module.__file__ as I showed above, by only making a small change to the generated command.

@wagnerluis1982
Copy link
Contributor

This issue was noticed because of Werkzeug's reloader, but it's not actually related to fixing that issue or being able to relaunch a process. The issue with Poetry is that it's changing sys.argv[0] to something unexpected.

Here you're overstating. Remember poetry run can run any command, not only python scripts. I compare to docker run command, if you ask to run, it will run. So, if I ask to run a script on the environment PATH (which is the case of installed script), I expect to behave like that, which poetry does well with the fix made on #6737. If you have your poetry on latest master, you're going to have this behavior.

The sys.argv[0] = "-c" matches Python's documented behavior for the python -c code invocation, so that's also an acceptable fix, although it wouldn't provide launch details.

It is never an acceptable fix, for the reason I mentioned above.

Fixing Poetry by making it use sys.argv[0] = "full path to module" would be best. That matches the behavior of python -m module, which Poetry is basically emulating with its entry point.

python -m relies on if __name__ == "__main__" in the module, poetry must not follow this. If you want it, you can simply go with poetry run python -m module.

@wagnerluis1982
Copy link
Contributor

The discussion here took a different path towards a different type of issue (if there is an issue at all). Since the issue originally reported by @delucca was fixed on #6737, I suggest the maintainers to close this as duplicate.

@wagnerluis1982
Copy link
Contributor

@davidism I recommend you to create a discussion (Discord or GitHub) with your point if you still feel that you need (or to create a new issue).

@delucca
Copy link
Author

delucca commented Feb 20, 2023

@wagnerluis1982 I think @davidism was able to create a workaround to handle it on Flask's side, but I honestly don't think this is the proper fix for that

I understand that poetry run is supposed to allow us handling anything (not only Python) that is currently in PATH, but this is counterproductive as soon as to do this we need to change Python execution in a way that damages the usability of Python scripts

For example, I have another PR that I think can be related to the same issue. On that issue, if we run a Python application without using Poetry we can successfully attach a debugger to it (using nvim-dap), but if we use Poetry it simply breaks as soon as the server launches

I'm still not 100% sure it is a related issue (I still need to debug it), but based on the current discussion I'm almost sure it is

@wagnerluis1982
Copy link
Contributor

@delucca

I understand that poetry run is supposed to allow us handling anything (not only Python) that is currently in PATH, but this is counterproductive as soon as to do this we need to change Python execution in a way that damages the usability of Python scripts

Are we still talking about Poetry 1.3.2 behavior? If so, a fix is already provided, only waiting for 1.4.0 release to happen (very soon).

For example, I have another PR that I think can be related to the same issue. On that issue, if we run a Python application without using Poetry we can successfully attach a debugger to it (using nvim-dap), but if we use Poetry it simply breaks as soon as the server launches

Here, while may be related to the design decisions of poetry run, we are talking of a different issue and should be part of a different issue.


For all the things, do you mind to try to test with a Poetry installed on latest master?

@wagnerluis1982
Copy link
Contributor

Hi @delucca, poetry 1.4.0 is released. Can you give a check with your use cases?

@delucca
Copy link
Author

delucca commented Mar 2, 2023

@wagnerluis1982 just tried here and the issue remains 😢

@delucca
Copy link
Author

delucca commented Mar 2, 2023

@davidism I also tried updating Flask to the latest version, but the issue persists

@davidism
Copy link

davidism commented Mar 2, 2023

That's because there's no new version of Werkzeug yet. But that shouldn't matter, if Poetry did add the full path to the Python file in sys.argv then it would start working.

@wagnerluis1982
Copy link
Contributor

@davidism but you have the full path to the entry point (once is installed).

If you want the full path to the module, you can simply do poetry run python -m module

@wagnerluis1982
Copy link
Contributor

@wagnerluis1982 just tried here and the issue remains 😢

You mean the issue described in the issue?

If so, did you install the entry point via poetry install?

@dimbleby
Copy link
Contributor

dimbleby commented Mar 2, 2023

argv[0] is the script name (it is operating system dependent whether this is a full pathname or not)

I could see a case for argv[0] being the link /some/virtualenv/bin/show, but /absolute/path/to/example.py doesn't seem right at all

if you first activate the environment and then just run show, that's what happens.

Edit: or anyway, that's what happens on my operating system. In view of the "operating system dependent" I'd suggest that it's unwise to rely on the full path being available anyway

@davidism
Copy link

davidism commented Mar 2, 2023

Looks like everything works correctly with Poetry 1.4.0.

from flask import Flask

app = Flask(__name__)

@app.get("/")
def index():
    return "Hello, Poetry!"

def dev_server():
    app.run(debug=True)
[tool.poetry.scripts]
dev_server = "example:dev_server"
$ poetry install

$ poetry run dev_server
 * Serving Flask app 'example'
 * Debug mode: on
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://127.0.0.1:5000
Press CTRL+C to quit
 * Restarting with stat
 * Debugger is active!

$ poetry run flask -A example run --debug

 * Serving Flask app 'example'
 * Debug mode: on
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://127.0.0.1:5000
Press CTRL+C to quit
 * Restarting with stat
 * Debugger is active!

Both commands work as expected and the reloader works correctly.

poetry install is required, otherwise sys.argv[0] is still ["dev_server"]. I still think that could be fixed by using sys.argv[0] = mod.__file__ in the command Poetry generates internally, but poetry install should be required anyway so it's up to you at this point.

@wagnerluis1982
Copy link
Contributor

I still think that could be fixed by using sys.argv[0] = mod.__file__ in the command Poetry generates internally, but poetry install should be required anyway so it's up to you at this point.

As said multiple times, makes no sense poetry run entry_point has a different behavior of poetry shell; entry_point

@davidism
Copy link

davidism commented Mar 2, 2023

When not installed, entry_point as a standalone script doesn't exist to be called within poetry shell. So right now, for uninstalled scripts, Poetry generates an outright incorrect sys.argv[0] by setting it to the name of the script. To be correct, it could set it to "-c", or to be less correct but more helpful it could set it to mod.__file__.

But as I said, this works for installed projects now, you're welcome to close it if you don't want to address uninstalled projects.

@wagnerluis1982
Copy link
Contributor

Adding to the discussion, I feel the wrongness feeling is being fueled by the fact that poetry run may have different behaviors regarding entry point (installed / non-installed).

So, I feel that poetry should forbid at all to run non installed entry points. What do maintainers think about it?

@radoering
Copy link
Member

At the moment, I don't see a use case why it should be necessary to run non installed entry points. However, before we forbid it we should deprecate it and see if people do complain.

@delucca
Copy link
Author

delucca commented Mar 3, 2023

@wagnerluis1982 just tried here and the issue remains 😢

You mean the issue described in the issue?

If so, did you install the entry point via poetry install?

oh, do I need to run poetry install again after adding the script line?

I'm running a installed entry point (the entry point of my app). I'll try it again to see if it works and let you know.

@delucca
Copy link
Author

delucca commented Mar 3, 2023

@wagnerluis1982 okay, I ran poetry install and now it works as expected 😄

Since I didn't add any new dependencies, I didn't know I had to run poetry install before testing.

So, I think it is fixed 😄

@wagnerluis1982
Copy link
Contributor

I open a discussion at #7599 to we use to decide what to do with poetry run with unistalled scripts.

@delucca
Copy link
Author

delucca commented Mar 3, 2023

@wagnerluis1982 perfect 😄

I think we can close this one

@radoering
Copy link
Member

Resolved for installed scripts in #6737. Discussion for non-installed scripts in #7599.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

6 participants