luisg123v commented 1 year ago

Such dependency is already included in Odoo requirements using a pinned version [1]. Adding here could cause to upgrade the library to an incompatible version.

[1] https://github.com/odoo/odoo/blob/54e58b3e47ee/requirements.txt#L51

luisg123v commented 1 year ago

Hi @moylop260, @guewen,

Could you review, please?

CC @randall-vx, @desdelinux

OCA-git-bot commented 1 year ago

Hi @guewen, some modules you are maintaining are being modified, check this out!

luisg123v commented 1 year ago

Hi @simahawk,

I understand this is a required library. However, this library is also installed by Odoo natively:

https://github.com/odoo/odoo/blob/54e58b3e47ee/requirements.txt#L51

So there's no need to add it also here, because, if we're using Odoo, we can know for sure it will be installed. Besides, as you may appreciate on the above link, library version varies depending on Python version, for compatibility reasons.

If we try to install requirements from here, we could be installing an incompatible version for Odoo.

Lastly, external dependencies are meant to be external packages not-already installed by Odoo. If we'd have to add requests on every module that required it, we would have to add it on a lot of website modules., and similarly, we would have to add python-dateutil on every module that deals with dates.

I hope my point is clear now.

Regards,

luisg123v commented 1 year ago

Hi @pedrobaeza, What do you think about this one?

sbidoul commented 1 year ago

I think it is important that each modules explicitly declares the dependencies it relies on. At the minimum it has documentation value. The fact that Odoo depends on request is a kind of implementation detail of Odoo and they could decide to use something else in the future. In that case we'll be happy to know which module depend on it. This is true for all dependencies.

@luisg123v could you elaborate in which cases this causes problems so we can better understand the reason for this PR?

pedrobaeza commented 1 year ago

If it's a problem to remove from here the requests dependency, is it OK for you to pin it here in the same version than the Odoo one?

sbidoul commented 1 year ago

If it's a problem to remove from here the requests dependency, is it OK for you to pin it here in the same version than the Odoo one?

That is even worse, because different environments may require different versions of the request library. Addons authors should not pin dependencies unless there are very good reason to do so. Pinning dependencies is the role of the integrator, not the role of library/addons authors, who should only place constraints on the versions that are necessary for their needs, not more.

luisg123v commented 1 year ago

Hi @sbidoul,

Adding a requirement already included by Odoo unpinned could cause several issues:

Since it's not pinned, an earlier (and possibly incompatible) version of the package could be installed when installing requirements of this repo. Odoo pins requirements for a reason, which is they can "guarantee" it will work correctly.
Starting from pip 20.3, the version resolution is performed using backtracking (bruteforce to find right candidates). That means, if we add several unpinned dependencies, we will be making that task much more harder. We've seen some customers whose resolution has lasted literally several hours, without even finding a right combination.

On the other hand, answering to this:

The fact that Odoo depends on request is a kind of implementation detail of Odoo and they could decide to use something else in the future.

It may be so, but this module is designed to work with this specific Odoo version. For this Odoo version, requests is not an external dependency, as it's included by Odoo. If in the next Odoo version they decide to use a different library, it will become an external dependency, but for the future version.

As I mentioned above, if we neeeded to specify all libraries used by our modules that are already part of Odoo, we will need for instance to include urllib in most of the theme modules.

I hope I made my point clear now.

Regards,

sbidoul commented 1 year ago

Odoo pins requirements for a reason, which is they can "guarantee" it will work correctly.

Sure they guarantee that. But it often happens that we need to deploy other libraries that are not compatible with the versions pinned in Odoo's requirements.txt (which are often outdated, to be honest). In that case it is the responsibility of the integrator to resolve the conflict and test accordingly.

Since it's not pinned, an earlier (and possibly incompatible) version of the package could be installed when installing requirements of this repo. Odoo pins requirements for a reason, which is they can "guarantee" it will work correctly.

Then, frankly, it is the deployment process that needs to be fixed. We do that daily and never had that kind of issue. When you pip install requests and another version is already installed, the already installed version takes precedence. And with any reasonably recent pip version, if you pip install request requests==2.25.1 it works just fine.

We've seen some customers whose resolution has lasted literally several hours, without even finding a right combination.

It that case it is probable that there is no solution. But it never happens with a dependency without version constraints like requests here. BTW, you should try using pip 23.1, which as significant improvements in reporting resolution errors faster.

If in the next Odoo version they decide to use a different library, it will become an external dependency, but for the future version.

Yes, and if we have not declared direct external dependencies we'll have no easy way to analyze and locate places where we need to change.

As I mentioned above, if we neeeded to specify all libraries used by our modules that are already part of Odoo, we will need for instance to include urllib in most of the theme modules.

Do you mean urllib from the python standard library? In that case, no, it is not an external dependency as it is part of Python itself.

For this Odoo version, requests is not an external dependency, as it's included by Odoo.

This is not the correct definition of external dependency. An external dependency is any library that is not part of the standard python library and is used directly by the addon.

To take another example, assume I write a python application that uses both requests and charset-normalizer. In the dependencies of my application I will declare both, because I don't want to rely on the fact that requests uses charset-normalizer under the hood. This is exactly the same for Odoo addons.

luisg123v commented 1 year ago

In that case it is the responsibility of the integrator to resolve the conflict and test accordingly.

I disagree on this. Suppose I want to implement a module related to product that is not compatible with the native product module. IMHO, it's responsibility of the module developer to make such module compatible with Odoo.

When you pip install requests and another version is already installed, the already installed version takes precedence.

This is true only if no upgrade is asked. If I run

pip install -U -r requirements.txt

On this repo, a version different from the one included by Odoo will be installed

It that case it is probable that there is no solution.

The solution is to pin or delimit valid versions. The problem with no delimiting versions is literally all available versions could be considered.

Yes, and if we have not declared direct external dependencies we'll have no easy way to analyze and locate places where we need to change.

We have a lint for that, to detect external dependencies not declared on manifest. BTW, on that lint, we have whitelisted Odoo requirements for the same reason, which is we consider them implicit ones.

Do you mean urllib from the python standard library? In that case, no, it is not an external dependency as it is part of Python itself.

You're right, that was not a good example. We could take instead the requests package.

This is not the correct definition of external dependency. An external dependency is any library that is not part of the standard python library and is used directly by the addon.

Where does that definition come from?

I don't agree with that definition. See Odoo modules, none of them include requests on their external dependencies.

To take another example, assume I write a python application that uses both requests and charset-normalizer. In the dependencies of my application I will declare both, because I don't want to rely on the fact that requests uses charset-normalizer under the hood.

I think this is debatable. When you reference dependencies that you already know will be installed, that is called superfluous dependencies.

This is exactly the same for Odoo addons.

Exactly. When we need to depend on the account module, we don't manually depend also on product, portal, analytic, etc; even if we require features from those modules, because we know they are required by the account module, so we can assure they will be installed. Then, those dependencies are superfluous.

sbidoul commented 1 year ago

This is true only if no upgrade is asked. If I run pip install -U -r requirements.txt On this repo, a version different from the one included by Odoo will be installed

This is really the part you need to fix in your workflow.

Asking pip to do an upgrade without giving the full requirements set is going to break your environment one day or another.

Imagine for instance that you need repo A that requires some-dependency and repo B that requires some-dependency<2. If you do pip install -U -r repo-A-requirements.txt you break your environment because pip does not know the full set of constraints when doing the upgrade. The only way to do a correct upgrade is to provide all the requirements to pip with -r or -c.

When we need to depend on the account module, we don't manually depend also on product, portal, analytic, etc; even if we require features from those modules, because we know they are required by the account module, so we can assure they will be installed.

Well, I do ;) If I inherit a view, I always depend on the lowest level module that defines the view with the features I need. And I try to do the same when I inherit a model. This give a much better understanding of the dependency graph.

luisg123v commented 1 year ago

Asking pip to do an upgrade without giving the full requirements set is going to break your environment one day or another.

I agree on this. Actually, this is why I think all requirements should be pinned or at least delimited, whenever possible. Otherwise, if I depend on several OCA repos and each of them provides several unpinned requirements and I build a single requirements file containing all of them, pip could fail to find the right combination of pip dependencies (the case I commented above).

Well, I do ;) If I inherit a view, I always depend on the lowest level module that defines the view with the features I need.

I disagree with this approach, this would force us to include a bunch of dependencies that are not necessary (superfluous ones). We even had a module that identified superfluous ones and linted them (I don't remember the repo, though, maybe server-tools).

This give a much better understanding of the dependency graph.

I also disagree on this. When Odoo computes the dependency graph, it builds something similar to a tree, i.e. by levels. When we include only non-superfluous dependencies, our graph is more similar to what Odoo builds. But if we include them, is much more harder to determine which module depends on what one.

Having a good notion of the actual module graph is useful in many situations, e.g. during migrations because we may know what modules to migrate at first.

desdelinux commented 1 year ago

@sbidoul I have replicated the problem in odoo.sh.

When placing this repo, the newest version is installed on top of the one that Odoo has pinned.

request is already in the odoo requirements, it shouldn't be as a dependency here.

moylop260 commented 1 year ago

I have seen a few projects with PRs already merged with similar changes except this one

Should we have special consideration in this project?

Also, It is installing non even compatible version py packages for deployments != OCA (e.g. OdooSH, DeployV)

simahawk commented 1 year ago

I have seen a few projects with PRs already merged with similar changes except this one

It does not demonstrated that it was the right thing to do :wink:

Should we have special consideration in this project?

No, we should fix the others where nobody considered this from a more pythonic POV.

Also, It is installing non even compatible version py packages for deployments != OCA (e.g. OdooSH, DeployV)

As reported above that depends on how you run pip install. I see that on odoo.sh you can pin your own versions w/ proper req.txt https://www.odoo.com/documentation/14.0/administration/odoo_sh/getting_started/first_module.html#use-an-external-python-library

When placing this repo, the newest version is installed on top of the one that Odoo has pinned.

Which command runs to install such dependencies?

BTW we pin dependencies on our projects, we don't rely on what you pin elsewhere.

moylop260 commented 1 year ago

@simahawk

Thank you for your reply my friend

It does not demonstrated that it was the right thing to do 😉

In fact, I was not talking about "the right thing to do"

I like to understand what is unique in this project to do it differently (not correct or wrong)

I mean, if we decide to do the same than this project for all OCA's projects so we should add a requirements file for all these projects with the same packages that Odoo has already considered in its requirements file

Project: account-analytic. Packages: psycopg2
Project: account-budgeting. Packages: dateutil
Project: account-closing. Packages: dateutil
Project: account-consolidation. Packages: dateutil, lxml
Project: account-financial-reporting. Packages: psycopg2
Project: account-financial-tools. Packages: dateutil
Project: account-fiscal-rule. Packages: lxml, requests
Project: account-invoicing. Packages: dateutil
Project: account-payment. Packages: dateutil, lxml
Project: account-reconcile. Packages: psycopg2
Project: agreement. Packages: lxml
Project: apps-store. Packages: lxml
Project: bank-payment. Packages: dateutil, stdnum, lxml
Project: bank-statement-import. Packages: pytz, dateutil, lxml, requests
Project: brand. Packages: lxml, markupsafe
Project: commission. Packages: psycopg2, dateutil, lxml
Project: connector. Packages: psycopg2
Project: connector-infor. Packages: psycopg2
Project: connector-jira. Packages: pytz, dateutil, psycopg2
Project: connector-telephony. Packages: requests
Project: contract. Packages: dateutil, lxml
Project: credit-control. Packages: dateutil
Project: crm. Packages: psycopg2
Project: currency. Packages: dateutil
Project: data-protection. Packages: lxml, werkzeug
Project: delivery-carrier. Packages: PIL, lxml, requests
Project: donation. Packages: psycopg2
Project: e-commerce. Packages: psycopg2, werkzeug
Project: edi. Packages: dateutil, werkzeug, pytz, lxml, PyPDF2, requests
Project: event. Packages: pytz, babel
Project: helpdesk. Packages: werkzeug
Project: hr. Packages: dateutil
Project: hr-attendance. Packages: pytz, dateutil, psycopg2
Project: hr-holidays. Packages: pytz, dateutil
Project: interface-github. Packages: docutils, requests
Project: intrastat-extrastat. Packages: dateutil, stdnum, lxml
Project: iot. Packages: jinja2
Project: l10n-belgium. Packages: dateutil
Project: l10n-brazil. Packages: dateutil, werkzeug, pytz, lxml, requests, pkg_resources
Project: l10n-chile. Packages: pytz, dateutil, requests
Project: l10n-estonia. Packages: lxml
Project: l10n-france. Packages: dateutil, reportlab, pytz, lxml, PyPDF2, requests
Project: l10n-germany. Packages: dateutil
Project: l10n-iran. Packages: pytz, babel
Project: l10n-italy. Packages: psycopg2, dateutil, openupgradelib, lxml
Project: l10n-netherlands. Packages: dateutil, psutil, lxml
Project: l10n-portugal. Packages: requests, werkzeug
Project: l10n-romania. Packages: pytz, dateutil, lxml, requests
Project: l10n-spain. Packages: dateutil, pytz, cryptography, lxml, requests
Project: l10n-switzerland. Packages: jinja2, lxml, PIL, PyPDF2, requests
Project: l10n-thailand. Packages: pytz, dateutil
Project: l10n-ukraine. Packages: pytz, dateutil
Project: l10n-usa. Packages: psycopg2, dateutil, stdnum
Project: l10n-vietnam. Packages: lxml
Project: maintenance. Packages: lxml
Project: management-system. Packages: psycopg2
Project: manufacture. Packages: psycopg2, dateutil
Project: mis-builder. Packages: pytz, dateutil, lxml
Project: odoo-pim. Packages: lxml
Project: partner-contact. Packages: dateutil, psycopg2, pytz, lxml, requests
Project: payroll. Packages: pytz, dateutil, babel
Project: pms. Packages: dateutil, pytz, babel, PyPDF2, requests
Project: product-attribute. Packages: psycopg2, dateutil, lxml
Project: product-configurator. Packages: lxml, mako
Project: product-variant. Packages: lxml
Project: project. Packages: pytz, dateutil, lxml, werkzeug
Project: purchase-workflow. Packages: dateutil
Project: queue. Packages: psycopg2, dateutil, lxml, werkzeug
Project: report-print-send. Packages: PIL, requests
Project: reporting-engine. Packages: dateutil, werkzeug, qrcode, psycopg2, lxml, PIL, requests, pkg_resources, pydot, xlsxwriter
Project: rest-framework. Packages: marshmallow, apispec, werkzeug
Project: runbot-addons. Packages: requests
Project: sale-workflow. Packages: dateutil, werkzeug, psycopg2, pytz, lxml
Project: search-engine. Packages: elasticsearch
Project: server-auth. Packages: ldap, werkzeug, passlib, jwt, requests
Project: server-backend. Packages: psycopg2
Project: server-brand. Packages: lxml
Project: server-env. Packages: lxml
Project: server-tools. Packages: dateutil, werkzeug, psycopg2, jinja2, lxml, xlwt, requests, xlrd
Project: server-ux. Packages: pytz, dateutil, lxml
Project: social. Packages: lxml, markupsafe, werkzeug, requests
Project: stock-logistics-barcode. Packages: dateutil, lxml, barcode
Project: stock-logistics-warehouse. Packages: psycopg2, reportlab, werkzeug
Project: stock-logistics-workflow. Packages: lxml
Project: storage. Packages: requests, werkzeug
Project: timesheet. Packages: dateutil, pytz, psycopg2, lxml, babel, xlsxwriter
Project: vertical-abbey. Packages: pytz, dateutil, cStringIO
Project: vertical-association. Packages: dateutil
Project: vertical-hotel. Packages: dateutil
Project: vertical-isp. Packages: requests
Project: vertical-medical. Packages: dateutil, urllib2
Project: vertical-rental. Packages: dateutil
Project: web. Packages: PIL
Project: web-api. Packages: requests, werkzeug
Project: website. Packages: lxml
Project: website-cms. Packages: psycopg2, werkzeug, mock
Project: wms. Packages: psycopg2, werkzeug

The question is: Why all these projects don't have these needed requirements following the same logic as queue project decisions?

Why queue project has this decision and it needs this special consideration?

simahawk commented 1 year ago

Hola Moises, hope you are doing fine mate :)

Sorry but I still don't understand while you think this is a special project. It's not. I guess it's because you miss the point below.

:information_source: Declaring direct dependencies of a module/package is a must have. It's a best practice. Period. :information_source: Pinning versions at project level is a best practice. Period.

Until you all fail to recognize these important things we'll discuss forever and ever on this PR.

Hence, if other OCA modules are doing the contrary we must fix them (unless strictly necessary I've never seen a precise pinning).

We'll be glad to have a list of PRs like this: can you get back to us w/ some examples?

Regarding

I mean, if we decide to do the same than this project for all OCA's projects so we should add a requirements file for all these projects with the same packages that Odoo has already considered in its requirements file

I do not understand this statement. It's not about OCA repos.

Both Stéphane and me already pointed out that if you rely on the wrong pip install workflow you'll run into troubles unless you pinned all fragile direct and transitive python deps in your own projects not in our repos.

Regarding "what's the possible solution to unstuck this?" Pinning specific versions is bad because it makes much easier to run into versions conflicts. A good compromise could be to find out and pin the right range of versions (maybe just the top or the bottom bound) that are compatible w/ v14 (in this case). BUT as I mentioned above, running into conflicts will be your next enemy. For these reasons everyone should pin versions according to proj needs and do not rely on pip install upgrades.

Hope this helps :)

moylop260 commented 1 year ago

Hi @simahawk

Yes, you are right

ℹ️ Pinning versions at project level is a best practice. Period.

If I understood well this point so we should have the following requirements too for this project:

# generated from manifests external_dependencies
requests
+ psycopg2
+ werkzeug

Because the code is using these packages directly?

https://github.com/OCA/queue/blob/f9abba54565afc80844ac30c527b25529e7de899/queue_job/controllers/main.py#L11-L12

Why they were not added but request?

pedrobaeza commented 1 year ago

I think the Python rules can't apply strictly to Odoo modules, as they are not the same. We are using Python, but inside an ORM framework provided by Odoo. Being both the libraries and versions set at Odoo level, and not changing inside the same version, requiring to repeat the definitions at Odoo modules level is a nonsense, as if you don't have such requirements, Odoo won't work and won't execute your Odoo module.

moylop260 commented 1 year ago

Hi @simahawk @sbidoul @pedrobaeza @luisg123v @rvalyi

ℹ️ Pinning versions at project level is a best practice. Period.

Notice the following packages are used directly?

https://github.com/OCA/queue/blob/f9abba54565afc80844ac30c527b25529e7de899/queue_job/controllers/main.py#L11-L12

Following the best practice here, Should we add these packages too?

# generated from manifests external_dependencies
requests
+ psycopg2
+ werkzeug

I'm trying to get an OCA guideline, we have the following opinions

Option A

1) Add all the requirements even if odoo has already defined and pinned 2) Odoo requirements should be unpinned even if odoo has pinned

Option B

  1) Add all the requirements even if odoo has already defined and pinned
- 2) Odoo requirements should be unpinned even if odoo has pinned
+ 2) Odoo requirements should be pinned because odoo has pinned too

Option C

1) No add the requirements that odoo has already defined and pinned because it depends on Odoo and Odoo depends on Odoo's requirements already installed

What option should we use for OCA guidelines?

Please, vote

sbidoul commented 1 year ago

Hi @moylop260,

Following the best practice here, Should we add these packages too?

Yes, I think psycopg2 and werkzeug should be added to the manifest of queue_job, since they are direct dependencies, if only for documentation purposes.

Regarding pinning, it is a responsibility of the integrator delivering the end-project.

This means that:

addons should never place exact pins on their dependencies (whether they are also Odoo dependencies or not);
addons may require a lower bound (e.g. somelib>1.4) if they depend on recent features of a library (but not higher than necessary);
addons may exceptionally place an upper bound if they know they are incompatible with recent versions of a library AND it is not feasible to fix the compatibility issue.

If too strict bounds or pinning is done in addons, then integrators quickly land in dependency hell with unresolvable depency conflicts.

Note that versions in Odoo's requirement.txt are a recommendation (known good versions), but Odoo does work perfectly well with more recent versions, and it frequently happens that one needs to use more recent versions of the Odoo requirements to be compatible with other modern libraries.

rvalyi commented 1 year ago

I agree with everything @sbidoul said. That being said, I take the opportunity to share a minor issue we found in l10n-brazil with this approach: if several repo addons depend on the same Python package and if some addon start to require some minimum version, then we should also repeat this minum version in the manifest of the other addons of the repo to avoid conflicts in the requirements.txt automatic generation. Really not a big deal, but anyhow I shared it with you.

sbidoul commented 1 year ago

@rvalyi actually, with recent (> ~21) versions of pip, it should not matter. A requirements file like this works:

somelib
somelib>1.4

rvalyi commented 1 year ago

@rvalyi actually, with recent (> ~21) versions of pip, it should not matter. A requirements file like this works:
somelib
somelib>1.4

we had it failed on the CI on 14.0 like a month ago. No idea how it is on 15.0 or 16.0 but good to know, thanks.

sbidoul commented 1 year ago

we had it failed on the CI on 14.0 like a month ago

Feel free to ping me if this happens again, I'll investigate.

CRogos commented 6 months ago

I did run into the same issue with odoo.sh, that there was a library update, which caused the library to be incompatible with the odoo code.

I do not see a good solution for odoo.sh users, yet. The fact is, that if we add OCA repos as submodules into odoo.sh, all requirements.txt are installed (even when the causing OCA module is not installed). This maybe not the best practice, but it is not in my hand to change odoo.sh and I think a lot of people using odoo.sh and the OCA repositories should be compatible with odoo.sh.

In the past I had 2 issues with incompatible libraries. One was "pillow" which was updated by one of my own modules, and the second one was "lxml" which had a recent update, which made it incompatible.

What do you think about to change the pre-commit job to not create a requirements.txt, but a oca-requirements.txt or repo-requirements.txt too bypass the auto install of odoo.sh?

Do I understand correctly that the dependencies in external_dependencies in the manifest is installed, when the module is activated? In this case the requirements.txt wouldn't be needed at all.

sbidoul commented 6 months ago

@CRogos if odoo.sh does not support this, that would be a good argument indeed.

By chance can you share an installation log that causes problems on odoo.sh? Because if you pip install requests when requests is already installed, pip will not upgrade it so it should be ok to have it in requirements.txt.

luisg123v commented 6 months ago

@CRogos,

Do I understand correctly that the dependencies in external_dependencies in the manifest is installed, when the module is activated? In this case the requirements.txt wouldn't be needed at all.

No, external dependencies are intended to block module installation if any dependency is missing, but not to autoinstall them.

CRogos commented 6 months ago

I recently had several issues. 1) social-pip.log social-pip.log When adding the newest version of social repo I get this error I don't use the module that causes this problem, but due to the requirement.txt file, it gets installed automatically.

2) warning-pip.log waring-pip.log The odoo.sh generated sh-requirements.txt In this case the installation is working, but I have some warnings in the logs. (Interesting that this does not cause odoo.sh to fail the build process. This is maybe an issue I might also address in an odoo ticket.)

3) lxml This error I saw recently, and therefore removed lxml from some oca repositories. This is not odoo.sh related but github workflow or docker related. (maybe this is also related to a bad workflow design in my setup) Nevertheless it was caused by an lxml change and a non-pinned version. lxml-pip.txt

4) pikepdf In another case I've simply added pikepdf as external dependency in one of my modules not knowing what I could cause... this updated pillow to version 10.1.0 (instead of 7.1) without complaining that it is not compatible with the odoo 16 version. In a result one Odoo native function was not compatible with the new library version and crashed when executed.

sbidoul commented 6 months ago

@CRogos thanks for posting those. For 1, 2, 4, yeah, welcome to dependency hell... these are painful and not easy to debug, and the best I can recommend to mitigate is https://github.com/OCA/queue/pull/530#issuecomment-1755968900. But for instance in case of 4, pikepdf requires Pillow >=10.0.1 so there is not much you can do if that version of pillow is incompatible with Odoo.

For 3 (lxml), it could be an issue with your workflow, but it could well be also something like the pillow issue above where some indirect dependency requires a more recent version of lxml than the one preinstalled in your CI.

CRogos commented 6 months ago

@sbidoul thanks again for your input. 1) there is already a PR available, but the project is currently not very active: https://github.com/decalage2/oletools/pull/812 4) I was able to pin pikepdf<8.5 which has not the pillow requirement and worked for me.

For 2+3 I have to think and test a little bit more. But there should be the option to added a upper boundary as soon as we find an incompatibility instead of removing it from the external_dependencies.

OCA / queue

[FIX] queue_job: remove `requests` from Python dependencies #530

Option A

Option B

Option C