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

Introduce non-package-mode #8650

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Conversation

radoering
Copy link
Member

@radoering radoering commented Nov 11, 2023

Pull Request Check List

Resolves: #1132

Requires: python-poetry/poetry-core#661

  • Added tests for changed code.
  • Updated documentation for changed code.

Introduce non-package mode

  • add a new attribute mode package-mode in tool.poetry
  • mode can be either package (default) or non-package
  • package-mode can be either true (default) or false
  • in non-package mode:
    • metadata like name and version is not required
    • the root package is never installed (same as --no-root)
    • building and publishing is not possible

Docs preview

Example Config:

[tool.poetry]
package-mode = false

[tool.poetry.dependencies]
python = "^3.8"
cleo = ">=2.0.0"

You can install this branch with pipx as follows:

pipx install --suffix _npm git+https://github.com/radoering/poetry.git@non-package-mode

This will install poetry with the suffix "_npm" so that you have to call poetry_npm instead of poetry. (The suffix is arbitrary, I chose "npm" as an abbreviation for non-package-mode. 😉)

@ilyagr

This comment was marked as resolved.

@ab
Copy link

ab commented Nov 13, 2023

Thanks for working on this! I tested it locally and everything worked as expected.

I know you left a TODO for docs, but it might be nice to update the help text in places like:

By default, the above command will also install the current project. To install only the
dependencies and not including the current project, run the command with the
<info>--no-root</info> option like below:
<info> poetry install --no-root</info>

@radoering radoering mentioned this pull request Nov 15, 2023
5 tasks
@radoering radoering added the impact/docs Contains or requires documentation changes label Nov 22, 2023
Copy link

github-actions bot commented Nov 22, 2023

Deploy preview for website ready!

✅ Preview
https://website-gvzf0kqv2-python-poetry.vercel.app

Built with commit 5f15049.
This pull request is being automatically deployed with vercel-action

docs/pyproject.md Outdated Show resolved Hide resolved
@Secrus
Copy link
Member

Secrus commented Nov 23, 2023

I wonder if we actually need to introduce that "mode" part. We could just have placeholder values for when the config is missing name and version and refuse to do packaging-related stuff (like artifact building) and project installation when those are not set properly. What do you think?

@mathewvaughan
Copy link

I wonder if we actually need to introduce that "mode" part. We could just have placeholder values for when the config is missing name and version and refuse to do packaging-related stuff (like artifact building) and project installation when those are not set properly. What do you think?

What you're saying might make sense for when those things aren't set, but it's not very explicit and relies on the user knowing implicitly that 'omitting name and version will activate non-package mode'.

Personally, I do use 'name' and 'version' even when I'm not building a package - and I feel like an explicit boolean setting of 'package=false' would be clearer.

@Secrus
Copy link
Member

Secrus commented Nov 23, 2023

Personally, I do use 'name' and 'version' even when I'm not building a package - and I feel like an explicit boolean setting of 'package=false' would be clearer.

Ok, so what use would you have for that mode if you are setting the name and version anyway?

@mathewvaughan
Copy link

Personally, I do use 'name' and 'version' even when I'm not building a package - and I feel like an explicit boolean setting of 'package=false' would be clearer.

Ok, so what use would you have for that mode if you are setting the name and version anyway?

It's the name and version of the thing I'm building, but it's not a package, it's a web service

@Secrus
Copy link
Member

Secrus commented Nov 23, 2023

It's the name and version of the thing I'm building, but it's not a package, it's a web service

Ok, but as you can see in PR description, other than removing the requirement for name and version, what that mode introduces is lock on building and publishing (which I would say is on the user to decide, no real need to hardlock it) and installation of the project is using --no-root by default (which I would say isn't that much effort to type in and we could have Poetry detect that name and version are placeholder values and default to --no-root anyway, with a proper message). So out of those three changes, I would say only the name/version is meaningful and for your case, it's already handled since you set it anyway.

@mathewvaughan
Copy link

It's the name and version of the thing I'm building, but it's not a package, it's a web service

Ok, but as you can see in PR description, other than removing the requirement for name and version, what that mode introduces is lock on building and publishing (which I would say is on the user to decide, no real need to hardlock it) and installation of the project is using --no-root by default (which I would say isn't that much effort to type in and we could have Poetry detect that name and version are placeholder values and default to --no-root anyway, with a proper message). So out of those three changes, I would say only the name/version is meaningful and for your case, it's already handled since you set it anyway.

I'm here because poetry kept asking me to specify 'no-root'. It's a nuisance to have to specify that every time I run install and also has no obvious semantic link to the concept of 'I'm actually not building a package, thanks'. I was merely providing an answer to your question 'Do we really need the mode part?' and I'm explaining why I think we do.

I can appreciate that blocking the publishing/building if there is no name/version would be sufficient to stop web services being published as packages, but as someone who uses poetry to build web services it would be nice for there to be an obvious way to configure poetry to not expect me to jump through hoops designed for package development.

I think mode isn't clear enough still, though. It feels too generic a term. I think it would be better to use package=true/false and default it to true during poetry init, providing an option to set it to false.

@radoering
Copy link
Member Author

I tend to say, it's good to have an explicit way because naming and versioning your project even if you don't intend to build is a legit use case (especially if we support PEP 621 in the future so that name and version are not poetry specific keys). So yes, at the moment it's only about --no-root or not, which can't be decided implicitly, but I think always having to add --no-root is annoyance enough. Further, I'm not sure whether other differences (which nobody is currently thinking about) might be added in the future.

However, we can think about making it a tri-state with the default (not explicitly set): package mode if name and version is defined, otherwise non-package mode. I'm just not sure if the little bit of comfort is worth making it more complicated (not only to implement but also to understand as a normal user)?

I think mode isn't clear enough still, though. It feels too generic a term. I think it would be better to use package=true/false and default it to true during poetry init, providing an option to set it to false.

I'm not sure if package is a good idea because we already have packages. Nevertheless, I'm open to suggestions for other names.

The option in poetry init makes sense (if we have an explicit setting) and might be added in this PR or later.

@mathewvaughan
Copy link

It's a fair concern to worry about collisions on the names, but whilst looking at how the packages option is used I've started to believe the similarity is actually quite natural. Thinking about how we want want these options to interact:

  • package=true and no packages? Then we look for a directory that has the correct package name.
  • package=true with packages? We simply respect the dirnames within packages
  • package=false with no packages. You're probably building a web service so we wont fuss about --no-root
  • package=false but you've configured packages? Go home, you're drunk. (And we maybe should warn/error here?)

I mean, even if we just went with package_mode=true/false? I'm just wondering if we otherwise open ourselves to having mode become a bloated config option.

The option in poetry init makes sense (if we have an explicit setting) and might be added in this PR or later.

I really would be curious what the lowest friction default choice would be for first-time setup? Either way, I've never contributed to an open source project before but --no-root has been bothering me at work so with your blessing I'd like to contribute here - Perhaps I could raise a separate PR with the poetry init changes, ready to be merged after this branch?

@srittau
Copy link
Contributor

srittau commented Dec 1, 2023

I would advise against inferring the mode from the presence or non-presence of certain fields. Not only is it against Python's motto of "explicit is better than implicit", it can also lead to hard-to-debug problems. Assume someone wants to build a package, but forgets or misspells one of the required fields: Things will not work as expected or be broken in subtle ways. But until you try to publish the package, poetry will pretend everything is fine, because from its standpoint it is.

@radoering
Copy link
Member Author

I'm quite convinced now that this should be an explicit option. This leaves naming as an open topic. Let's try to get a picture of the general mood. (I have no idea how well this will work, but it's worth a try. 😅)

Please react with the respective emoji if you have an opinion:

  • 🚀 mode with values "package" (default) and "non-package"
  • 🎉 package with values true (default) and false (consider that there is already packages, which however will only be used in package mode)
  • ❤️ package-mode with values true (default) and false
  • 👀 if you are waiting for this feature but do not care about the name

If you have two favorites and dislike the third option, just vote for your two favorites.

Disclaimer: This vote does not result in a binding decision. We may choose another name if we think there is a good reason.

@branchvincent
Copy link
Member

consider that there is already packages, which however will only be used in package mode

I'll offer one more option 😅 : packages = false

It re-uses the existing field, avoids the possibility of conflicting settings like package = False and packages = [...], and also mirrors cargo's publish field (which can be false or an array of packages to publish)

@radoering
Copy link
Member Author

I'll offer one more option 😅 : packages = false

It re-uses the existing field, avoids the possibility of conflicting settings like package = False and packages = [...], and also mirrors cargo's publish field (which can be false or an array of packages to publish)

One thing to consider: packages = [] might make sense to define a meta package without modules/packages but only dependencies. (Afaik that does not work at the moment and I'm not sure if that's a valid use case but I'd assume it is until the opposite is proven.) So packages = [] and packages = false would have a different meaning which might be difficult to understand?

@silverwind
Copy link

silverwind commented Dec 4, 2023

I too would prefer to just re-use the existing packages field where any empty/falsy value is taken as "there are no packages", e.g. [], False, None should all result in non-package mode.

Imho, this non-package mode should also be the default when packages is not specified.

@johnthagen
Copy link
Contributor

johnthagen commented Dec 5, 2023

Imho, this non-package mode should also be the default when packages is not specified.

I think this would break simple projects that only have a single package that is implicitly discovered. For example, the following repo builds a package, but it is implicitly found in src/fact based on convention and the fact name.

@henrysingleton
Copy link

Is there a reason you'd default to putting it in package mode? I would have thought Poetry would get more use as a package manager rather than a packaging tool, but I don't have much experience on the packaging side.

@radoering
Copy link
Member Author

Is there a reason you'd default to putting it in package mode?

To avoid a breaking change.

I would have thought Poetry would get more use as a package manager rather than a packaging tool, but I don't have much experience on the packaging side.

I do not think there is any reliable data thats tells us what percentage of users uses Poetry only for dependency management and not for packaging. This means we don't even know if a breaking change would make sense for the majority of users.

@radoering radoering force-pushed the non-package-mode branch 2 times, most recently from 5f09476 to 81f0504 Compare December 9, 2023 10:31
@radoering
Copy link
Member Author

FYI: I switched the PR from mode to package-mode as this seems to be more popular. Further, with no prospect of introducing further modes a Boolean option is sufficient.

@radoering radoering mentioned this pull request Dec 9, 2023
@johnthagen
Copy link
Contributor

building and publishing is not possible

A nice side effect of this change will be that for non-packages, we will also get a clean solution for private-ness as well 🚀

@nextmat
Copy link

nextmat commented Jan 22, 2024

Thanks for all the work on this all, I'm really excited to see this get merged. 😁

It looks like this is scheduled to be part of the 1.8 release. Reviewing the 1.8 issue it seems like the related PRs are mostly ready to go, I was curious about likely timeline to the release or if there is anything we can do to help?

@radoering
Copy link
Member Author

it seems like the related PRs are mostly ready to go

That's right. They are mainly waiting for the poetry-core release. I'm just waiting for some reviews before creating the release PR. After the poetry-core release we can do the final adjustments in the open PRs mentioned in #8770.

```toml
[tool.poetry]
package-mode = false
```

Choose a reason for hiding this comment

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

Will the setting work in poetry.toml as well? If so, I would add an example for that as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Actually, I don't think it would be a good idea to put it in poetry.toml because it's clearly a project setting and not a user setting. (It's not like user A wants to have this setting for project X while user B does not want this setting for project X.)

Copy link

@silverwind silverwind Feb 5, 2024

Choose a reason for hiding this comment

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

I meant poetry.toml inside a project directory, which is meant to hold project-specific poetry configuration as per docs:

Your local configuration of Poetry application is stored in the poetry.toml file, which is separate from pyproject.toml.

In any case, both variants are fine to me, and I guess this setting will only work in the tool.poetry section then. It even seems that tool.poetry is kind of a mislabeled thing because it does not configure poetry.

@Secrus Secrus added the status/waiting-on-core Requires changes to poetry-core first label Jan 30, 2024
@radoering radoering removed the status/waiting-on-core Requires changes to poetry-core first label Feb 3, 2024
@radoering radoering marked this pull request as ready for review February 3, 2024 10:14
@radoering radoering requested a review from a team February 5, 2024 16:59
Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

LGTM

- metadata like `name` and `version` is not required
- the root package is never installed (same as `--no-root`)
- building and publishing is not possible
@radoering radoering merged commit f49f3ee into python-poetry:master Feb 13, 2024
33 checks passed
@armanjal
Copy link

armanjal commented Feb 27, 2024

Thanks for this update.
When package-mode=false, poetry add ... does not seem to work. The error 'Key "name" does not exist.' pops up.
This command works when package-mode is enabled and the required fields are specified under [tool.poetry]

@johnthagen
Copy link
Contributor

johnthagen commented Feb 27, 2024

@armanjal It's generally best to open a new issue to report a problem like this. You can still reference this MR using #8650.

@armanjal
Copy link

@armanjal It's generally best to open a new issue to report an issue like this. You can still reference this MR using #8650.

Sure , thanks!

@thinh9e
Copy link

thinh9e commented Mar 13, 2024

Can we add a new parameter in poetry new and poetry init commands so that we can set it to false when creating a new project?

@neersighted
Copy link
Member

Please open a feature request issue -- merged PRs are not a good venue for discussion, as both initial comments and any follow-up are easy to lose track of in the noise. Creating a new issue, with a descriptive title and body both makes clear what the current (and ideally only) topic of discussion is, and makes returning to the conversation in the future easier.

@python-poetry python-poetry locked as resolved and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for application-first, library-first, and non-package modes that change Poetry defaults