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

Provide PEP-685-compliant extras #10481

Closed
wants to merge 4 commits into from
Closed

Conversation

mattoberle
Copy link

Description

pip==23.3.0 introduces a new "canonical" lookup behavior for package extras:

As a result, a command like this:

pip install 'sqlalchemy[postgresql_psycopg2binary]==1.4.47'

Results in a warning like this:

WARNING: sqlalchemy 1.4.47 does not provide the extra 'postgresql-psycopg2binary'

The extra eventually resolves when pip falls back to the original postgresql_psycopg2binary.
This PR proposes the addition of the PEP-685-compliant names to setup.cfg.

  • Maintains backward compatibility for pip<23.3.0
  • Avoids reliance on the "fallback" behavior of pip>=23.3.0

Checklist

Note: I'm not sure how to categorize this given the options in the PR template.
This is a packaging change.

This pull request is:

  • A documentation / typographical / small typing error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

`pip==23.3.0` introduces a new "canonical" lookup behavior for package extras:

- https://pip.pypa.io/en/stable/news/#v23-3
- pypa/pip#12002

As a result, a command like this:

```
pip install 'sqlalchemy[postgresql_psycopg2binary]==1.4.47'
```

Results in a warning like this:

```
WARNING: sqlalchemy 1.4.47 does not provide the extra 'postgresql-psycopg2binary'
```

The extra eventually resolves when pip falls back to the original `postgresql_psycopg2binary`.
Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

new pep not even two years ago already generating warnings...OK

here's what I want to do here:

  1. can you confirm postgresql-psycopg2binary is the correct format (e.g. "normalized")
  2. in the change here, the dashed names should be the primary names, as we will eventually remove the underscore names:
mssql-pyodbc = pyodbc
mssql_pyodbc =
    %(mssql-pyodbc)s

hoping the dash in the variable name there is accepted

  1. put all the "legacy" underscore names in a separate batch way at the bottom so they are out of the way:
# legacy pre-pep-685 names
mssql_pymssql =
   %(mssql-pymssql)
mssql_pyodbc =
   %(mssql-pyodbc)
mysql_connector = 
   %(mysql-connector)

# ...

use the new names in our tox.ini file in the "extras" section:

diff --git a/tox.ini b/tox.ini
index ed68dbfcf7..675b80186b 100644
--- a/tox.ini
+++ b/tox.ini
@@ -18,20 +18,20 @@ extras=
      sqlite_file: aiosqlite
      sqlite_file: sqlcipher; python_version < '3.10'
      postgresql: postgresql
-     postgresql: postgresql_asyncpg
-     postgresql: postgresql_pg8000
-     postgresql: postgresql_psycopg
+     postgresql: postgresql-asyncpg
+     postgresql: postgresql-pg8000
+     postgresql: postgresql-psycopg
 
      mysql: mysql
      mysql: pymysql
      mysql: asyncmy
      mysql: aiomysql
-     mysql: mariadb_connector
+     mysql: mariadb-connector
 
      oracle: oracle
      oracle: oracle_oracledb
      py{3,37,38,39,310,311}-mssql: mssql
-     py{3,37,38,39,310,311}-mssql: mssql_pymssql
+     py{3,37,38,39,310,311}-mssql: mssql-pymssql
 
 install_command=
      # TODO: I can find no way to get pip / tox / anyone to have this

with all that we can run through gerrit make sure it all works

thanks!

@zzzeek
Copy link
Member

zzzeek commented Oct 16, 2023

ok,lets try it

@CaselIT any comments?

@zzzeek zzzeek requested a review from sqla-tester October 16, 2023 18:54
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 2431850 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 2431850: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4906

@mattoberle
Copy link
Author

Thanks for taking a look!


1. can you confirm `postgresql-psycopg2binary` is the correct format (e.g. "normalized")

The change to pip relies on a function called canonicalize_name:

Defined here:

That follows the PEP which defines the canonical name as:

re.sub(r"[-_.]+", "-", name).lower()

2. in the change here, the dashed names should be the primary names, as we will eventually remove the underscore names:

...
hoping the dash in the variable name there is accepted

This was my concern as well, but I made the change and it looks like %(var-with-dash)s is acceptable.


3. put all the "legacy" underscore names in a separate batch way at the bottom so they are out of the way:

...
use the new names in our tox.ini file in the "extras" section:

Done & done.

@CaselIT
Copy link
Member

CaselIT commented Oct 16, 2023

k,lets try it

@CaselIT any comments?

we must ensure that the fallback also works when using pyproject, an in the PR here #9324

also 2.1+ only or it's good also for 2.0?

@sqla-tester
Copy link
Collaborator

Federico Caselli (CaselIT) wrote:

we have to verify that the fallback works with PEP 621 (#9324). If not we have to find an alternative

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4906

@zzzeek
Copy link
Member

zzzeek commented Oct 16, 2023

k,lets try it
@CaselIT any comments?

we must ensure that the fallback also works when using pyproject, an in the PR here #9324

well this is the official syntax apparently, so it has to work in pyproject and everywhere else

also 2.1+ only or it's good also for 2.0?

it looks like 1.4 users are getting this warning already so if this is safe to do for 2.0 at least, that would be good

@CaselIT
Copy link
Member

CaselIT commented Oct 16, 2023

k,lets try it
@CaselIT any comments?

we must ensure that the fallback also works when using pyproject, an in the PR here #9324

well this is the official syntax apparently, so it has to work in pyproject and everywhere else

the pyformat thing %(name)s look a lot like something custom of cfg. As far as I know it's not supported in toml files.

what I'm saying is just to verify that this syntax works also when using toml, if not we can think of a workaround (likely just duplicating the actual requirements) and leave a note in the other PR mentioning that it is something to do before merging that one

@zzzeek
Copy link
Member

zzzeek commented Oct 16, 2023

wouldnt the toml use quoted names to handle identifier characters?

@CaselIT
Copy link
Member

CaselIT commented Oct 16, 2023

wouldnt the toml use quoted names to handle identifier characters?

checked out #9324
trying to use

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = %(postgresql-psycopg)s

then running pip install -e .[postgresql_psycopg] makes pip raise pip._vendor.tomli.TOMLDecodeError: Invalid value so that does not work.

trying

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = "%(postgresql-psycopg)s"

makes pip raise configuration error: `project.optional-dependencies.postgresql_psycopg` must be array

trying

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = [".[postgresql-psycopg]"]

or

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = ["%(postgresql-psycopg)s"]

raises instead configuration error: `project.optional-dependencies.postgresql_psycopg[0]` must be pep508

suggestions?

@zzzeek
Copy link
Member

zzzeek commented Oct 16, 2023

doesnt make much sense if the two peps conflict? there has to be some simple way to do this

@CaselIT
Copy link
Member

CaselIT commented Oct 16, 2023

doesnt make much sense if the two peps conflict? there has to be some simple way to do this

the problem is the alias. There isn't any problem is we do

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = ["psycopg>=3.0.7"]

If we find no way of defining an alias that would work too, but should be noted in the other PR

@zzzeek
Copy link
Member

zzzeek commented Oct 16, 2023

OK then we just define the a-b and a_b names direct to the thing they reference

the goal is:

a-b names point to their thing

a_b names are down at the bottom, pointing to whatever

@CaselIT
Copy link
Member

CaselIT commented Oct 16, 2023

works for me. will leave a note on the other PR

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

@CaselIT just to confirm you want to change these names right now even though we're not on pyproject.toml yet, right? maybe we can just wait until that time comes to change it further?

@@ -82,6 +82,30 @@ aiosqlite =
sqlcipher =
sqlcipher3_binary

# legacy pre-pep-685 names
mariadb_connector =
Copy link
Member

Choose a reason for hiding this comment

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

so, towards us being able to move to a pyproject.toml file easily, the request here is to not use the aliases for these legacy names and just point them to their ultimate dependencies directly (yes, duplicating what we have up above).

the goal here is that this batch of names is moved out to the margins where it can be removed at some point (maybe even 2.1, but maybe 2.2, etc) all at once.

Copy link
Author

Choose a reason for hiding this comment

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

What about items like postgresql-asyncpg?
Prior to this PR that was written as:

postgresql_asyncpg =
    %(asyncio)s
    asyncpg

Do we want this?

postgresql_asyncpg =
    %(asyncio)s
    asyncpg
postgresql-asyncpg =
    %(asyncio)s
    asyncpg

Or this?

postgresql-asyncpg =                                                             
    greenlet!=0.4.17                                                          
    asyncpg
postgresql_asyncpg =                                                             
    greenlet!=0.4.17                                                              
    asyncpg

%(asyncio)s is also referenced in 3 "single-word" extras:

  • aiomysql
  • asyncmy
  • aiosqlite

Copy link
Member

Choose a reason for hiding this comment

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

so from my point of view we can delay all the "remove the alias" to the PR that migrates to pyproject. As far as I know the toml version does not support aliases, but maybe there is a way that I'm not aware of

@CaselIT
Copy link
Member

CaselIT commented Oct 17, 2023

@CaselIT just to confirm you want to change these names right now even though we're not on pyproject.toml yet, right? maybe we can just wait until that time comes to change it further?

fine either way for me, I've left a comment in the other PR to do that change so we can delay it until then!

@CaselIT
Copy link
Member

CaselIT commented Oct 17, 2023

maybe hope is not lost: pypa/setuptools#3214 (comment)

if I understand correctly we can use (I haven't tried it)

postgresql-psycopg = ["psycopg>=3.0.7"]
postgresql_psycopg = ["sqlalchemy[postgresql-psycopg]"]

@zzzeek
Copy link
Member

zzzeek commented Oct 24, 2023

I'm still on the fence if we should be looking at this for 2.0. it seems a little soon, we could instead move right to 2.1 with pyproject.toml and just do the whole thing once. thoughts?

@CaselIT
Copy link
Member

CaselIT commented Oct 24, 2023

that works too for me

@CaselIT CaselIT added this to the 2.1 milestone Oct 24, 2023
@CaselIT
Copy link
Member

CaselIT commented Dec 8, 2023

This change here will be merged together with #9324, since it seems that when defined in pyproject a the extra foo-bar is automatically used when installing [foo_bar] and having only an extra foo_bar does not seem to work, at least in tox

@sqla-tester
Copy link
Collaborator

Federico Caselli (CaselIT) wrote:

will be merged with https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5028 since otherwise tox foes not work

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4906

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4906 has been abandoned. That means that at least for the moment I need to close this pull request. Sorry it didn't work out :(

@sqla-tester sqla-tester closed this Dec 8, 2023
@CaselIT CaselIT reopened this Dec 8, 2023
CaselIT pushed a commit to CaselIT/sqlalchemy that referenced this pull request Dec 8, 2023
Move the metadata for `setuptools` into `PEP 621`-compliant `pyproject.toml`.
Use PEP-685 extras, keeping the old names for backward compatibility.

Closes: sqlalchemy#9324
Closes: sqlalchemy#10481
Pull-request: sqlalchemy#9324
Change-Id: I14170e33a4a7370257d941adea4f96a39e785911
@CaselIT
Copy link
Member

CaselIT commented Feb 26, 2024

This was never backported to 2.0

I don't remember if we said it should

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.

4 participants