[PATCH net] tools: ynl: add the Python requirements.txt file

[PATCH net] tools: ynl: add the Python requirements.txt file

[Michal Michalik]

It is a good practice to state explicitely which are the required Python
packages needed in a particular project to run it. The most commonly
used way is to store them in the `requirements.txt` file*.

*URL: https://pip.pypa.io/en/stable/reference/requirements-file-format/

Currently user needs to figure out himself that Python needs `PyYAML`
and `jsonschema` (and theirs requirements) packages to use the tool.
Add the `requirements.txt` for user convenience.

How to use it:
1) (optional) Create and activate empty virtual environment:
  python3.X -m venv venv3X
  source ./venv3X/bin/activate
2) Install all the required packages:
  pip install -r requirements.txt
    or
  python -m pip install -r requirements.txt
3) Run the script!

The `requirements.txt` file was tested for:
* Python 3.6
* Python 3.8
* Python 3.10

Signed-off-by: Michal Michalik <michal.michalik@intel.com>
---
 tools/net/ynl/requirements.txt | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 tools/net/ynl/requirements.txt

diff --git a/tools/net/ynl/requirements.txt b/tools/net/ynl/requirements.txt
new file mode 100644
index 0000000..2ad25d9
--- /dev/null
+++ b/tools/net/ynl/requirements.txt
@@ -0,0 +1,7 @@
+attrs==22.2.0
+importlib-metadata==4.8.3
+jsonschema==4.0.0
+pyrsistent==0.18.0
+PyYAML==6.0
+typing-extensions==4.1.1
+zipp==3.6.0
-- 
2.9.5

base-commit: bcc858689db5f2e5a8d4d6e8bc5bb9736cd80626

Re: [PATCH net] tools: ynl: add the Python requirements.txt file

[Jakub Kicinski]

On Tue, 14 Mar 2023 17:07:58 +0100 Michal Michalik wrote:
> It is a good practice to state explicitely which are the required Python
> packages needed in a particular project to run it. The most commonly
> used way is to store them in the `requirements.txt` file*.
> 
> *URL: https://pip.pypa.io/en/stable/reference/requirements-file-format/
> 
> Currently user needs to figure out himself that Python needs `PyYAML`
> and `jsonschema` (and theirs requirements) packages to use the tool.
> Add the `requirements.txt` for user convenience.
> 
> How to use it:
> 1) (optional) Create and activate empty virtual environment:
>   python3.X -m venv venv3X
>   source ./venv3X/bin/activate
> 2) Install all the required packages:
>   pip install -r requirements.txt
>     or
>   python -m pip install -r requirements.txt
> 3) Run the script!
> 
> The `requirements.txt` file was tested for:
> * Python 3.6
> * Python 3.8
> * Python 3.10

Is this very useful? IDK much about python, I'm trying to use only
packages which are commonly installed on Linux systems. jsonschema
is an exception, so I've added the --no-schema option to cli.py to
avoid it.

> diff --git a/tools/net/ynl/requirements.txt b/tools/net/ynl/requirements.txt
> new file mode 100644
> index 0000000..2ad25d9
> --- /dev/null
> +++ b/tools/net/ynl/requirements.txt
> @@ -0,0 +1,7 @@
> +attrs==22.2.0
> +importlib-metadata==4.8.3
> +jsonschema==4.0.0
> +pyrsistent==0.18.0
> +PyYAML==6.0
> +typing-extensions==4.1.1
> +zipp==3.6.0

Why the == signs? Do we care about the version of any of these?
Also, there's a lot more stuff here than I thought I'm using.
What's zipp and why typing? Did I type something and forgot? :S

RE: [PATCH net] tools: ynl: add the Python requirements.txt file

[Michalik, Michal]

From: Jakub Kicinski <kuba@kernel.org> 
Sent: Thursday, March 16, 2023 5:40 AM
> 
> On Tue, 14 Mar 2023 17:07:58 +0100 Michal Michalik wrote:
>> It is a good practice to state explicitely which are the required Python
>> packages needed in a particular project to run it. The most commonly
>> used way is to store them in the `requirements.txt` file*.
>> 
>> *URL: https://pip.pypa.io/en/stable/reference/requirements-file-format/
>> 
>> Currently user needs to figure out himself that Python needs `PyYAML`
>> and `jsonschema` (and theirs requirements) packages to use the tool.
>> Add the `requirements.txt` for user convenience.
>> 
>> How to use it:
>> 1) (optional) Create and activate empty virtual environment:
>>   python3.X -m venv venv3X
>>   source ./venv3X/bin/activate
>> 2) Install all the required packages:
>>   pip install -r requirements.txt
>>     or
>>   python -m pip install -r requirements.txt
>> 3) Run the script!
>> 
>> The `requirements.txt` file was tested for:
>> * Python 3.6
>> * Python 3.8
>> * Python 3.10
> 
> Is this very useful? IDK much about python, I'm trying to use only
> packages which are commonly installed on Linux systems. jsonschema
> is an exception, so I've added the --no-schema option to cli.py to
> avoid it.
> 

Well, that is a very subjective question - I will present you my point of
view and leave a decision if that is useful or not to you. I'm totally
okay with rejecting this patch if you decide so.

First of all, I love the idea behind this script - thanks for creating it.
As a experienced Python user I looked at the scripts and see no file with
requirements so I assumed the default libs would be enough. First run
proved me wrong, exception on `jsonschema` missing*. No big deal, I
installed it. Second run, exception on `import yaml`. Still, no big deal
because I know that most probably you meant `PyYAML`. I don't really like
the idea that user need to either have this knowledge or look for it, so
I proposed this good practice file containing the required libs.

If you asked me, I feel it's useful - but again, I'm really okay to drop
this patch.

*yes, you added `no-schema` and `schema` with no defaults, it would be
enough to leave only `schema` defaulting to `None` or empty string and
users would not ran into the problem I faced, because right now default
behavior is to need this package

>> diff --git a/tools/net/ynl/requirements.txt b/tools/net/ynl/requirements.txt
>> new file mode 100644
>> index 0000000..2ad25d9
>> --- /dev/null
>> +++ b/tools/net/ynl/requirements.txt
>> @@ -0,0 +1,7 @@
>> +attrs==22.2.0
>> +importlib-metadata==4.8.3
>> +jsonschema==4.0.0
>> +pyrsistent==0.18.0
>> +PyYAML==6.0
>> +typing-extensions==4.1.1
>> +zipp==3.6.0
> 
> Why the == signs? Do we care about the version of any of these?
> Also, there's a lot more stuff here than I thought I'm using.
> What's zipp and why typing? Did I type something and forgot? :S
> 

I cannot (you probably also not) guarantee the consistency of the API of
particular libraries. For example, maybe somebody would change the calls
in `jsonschema 5.0.0` in the future. In this case - your script will
still work (will use 4.0.0). I suggested this packages versions, because
I checked them for Python 3.6, 3.8, 3.10. I'm fine with removing the
exact versions, but please don't blame me in the future if any of the
maintainters of those libs changes their API.

No, you did not forget about anything (besides the PyYAML that you didn't
mention above). There is more than you expect because `PyYAML` and
`jsonschema` have their own dependencies. If you like experimenting,
please create clean environment then do `pip list` and see the list,
then install only `PyYAML` and `jsonschema` and print the list again.

Re: [PATCH net] tools: ynl: add the Python requirements.txt file

[Edward Cree]

On 20/03/2023 19:03, Michalik, Michal wrote:
> From: Jakub Kicinski <kuba@kernel.org> 
>> Why the == signs? Do we care about the version of any of these?
> 
> I cannot (you probably also not) guarantee the consistency of the API of
> particular libraries.

Assuming the libraries are following best practice for their version
 numbering (e.g. semver), you should be able to use ~= ('compatible
 version' [1]).
For example, `jsonschema ~= 4.0` will allow any 4.x.y release, but
 not 5.0.0 since that could have breaking API changes.
I would recommend against pinning to a specific version of a
 dependency; this is a development tree, not a deployment script.

> No, you did not forget about anything (besides the PyYAML that you didn't
> mention above). There is more than you expect because `PyYAML` and
> `jsonschema` have their own dependencies.

Again I'd've thought it's better to let those packages declare their
 own dependencies and rely on pip to recursively resolve and install
 them.  Both on separation-of-concerns grounds and also in case a
 newer version of a package changes its dependencies.
(Probably in the past pinning all dependencies at the top level was
 needed to work around pip's lack of conflict resolution, but this
 was fixed in pip 20.3 [2].)

-ed

[1]: https://peps.python.org/pep-0440/#compatible-release
[2]: https://pip.pypa.io/en/latest/user_guide/#changes-to-the-pip-dependency-resolver-in-20-3-2020

RE: [PATCH net] tools: ynl: add the Python requirements.txt file

[Michalik, Michal]

On 20 March 2023 11:16 PM CET, Edward Cree wrote:
> 
> On 20/03/2023 19:03, Michalik, Michal wrote:
>> From: Jakub Kicinski <kuba@kernel.org> 
>>> Why the == signs? Do we care about the version of any of these?
>> 
>> I cannot (you probably also not) guarantee the consistency of the API of
>> particular libraries.
> 
> Assuming the libraries are following best practice for their version
>  numbering (e.g. semver), you should be able to use ~= ('compatible
>  version' [1]).
> For example, `jsonschema ~= 4.0` will allow any 4.x.y release, but
>  not 5.0.0 since that could have breaking API changes.
> I would recommend against pinning to a specific version of a
>  dependency; this is a development tree, not a deployment script.
>

This is actually a good idea. Let's wait for Jakub to confirm if he feels
the Python requirements file is a good idea in this case. If he confirms,
I would update the libraries according to your suggestion. Thanks.

>> No, you did not forget about anything (besides the PyYAML that you didn't
>> mention above). There is more than you expect because `PyYAML` and
>> `jsonschema` have their own dependencies.
> 
> Again I'd've thought it's better to let those packages declare their
>  own dependencies and rely on pip to recursively resolve and install
>  them.  Both on separation-of-concerns grounds and also in case a
>  newer version of a package changes its dependencies.
> (Probably in the past pinning all dependencies at the top level was
>  needed to work around pip's lack of conflict resolution, but this
>  was fixed in pip 20.3 [2].)

I agree with the comment that it is not a deployment script and that we
could leave only necessary packages and let pip do the rest of the job.
I will update if Jakub would decide he want to see the requirements here.

Thanks Edward for valuable input!

BR,
M^2

> 
> -ed
> 
> [1]: https://peps.python.org/pep-0440/#compatible-release
> [2]: https://pip.pypa.io/en/latest/user_guide/#changes-to-the-pip-dependency-resolver-in-20-3-2020

Re: [PATCH net] tools: ynl: add the Python requirements.txt file

[Jakub Kicinski]

On Tue, 21 Mar 2023 12:34:50 +0000 Michalik, Michal wrote:
> > Assuming the libraries are following best practice for their version
> >  numbering (e.g. semver), you should be able to use ~= ('compatible
> >  version' [1]).
> > For example, `jsonschema ~= 4.0` will allow any 4.x.y release, but
> >  not 5.0.0 since that could have breaking API changes.
> > I would recommend against pinning to a specific version of a
> >  dependency; this is a development tree, not a deployment script.
> 
> This is actually a good idea. Let's wait for Jakub to confirm if he feels
> the Python requirements file is a good idea in this case. If he confirms,
> I would update the libraries according to your suggestion. Thanks.

Given the "system script" nature of the project (vs "full application")
I don't find the requirements to be necessary right now. But I don't
know much about Python, so maybe Ed can make a call? :D

Re: [PATCH net] tools: ynl: add the Python requirements.txt file

[Edward Cree]

On 21/03/2023 17:52, Jakub Kicinski wrote:
> Given the "system script" nature of the project (vs "full application")
> I don't find the requirements to be necessary right now.

I'd say it's good to document the dependencies, because otherwise
 getting it to run could be a PITA for the user; poking around I
 don't see a convenient readme that could have a note added like
 "This tool requires the PyYAML and jsonschema libraries".
And if you're going to add a document just for this then it *may
 as well* be in the machine-readable format that pip install can
 consume.

> But I don't know much about Python, so maybe Ed can make a call? :D

I'm not exactly an expert either :D

RE: [PATCH net] tools: ynl: add the Python requirements.txt file

[Michalik, Michal]

On 21 March 2023 7:52 PM CET, Edward Cree wrote:
> 
> On 21/03/2023 17:52, Jakub Kicinski wrote:
>> Given the "system script" nature of the project (vs "full application")
>> I don't find the requirements to be necessary right now.
> 
> I'd say it's good to document the dependencies, because otherwise
>  getting it to run could be a PITA for the user; poking around I
>  don't see a convenient readme that could have a note added like
>  "This tool requires the PyYAML and jsonschema libraries".
> And if you're going to add a document just for this then it *may
>  as well* be in the machine-readable format that pip install can
>  consume.
> 

Thanks - I will prepare the new version which implements your suggestions
to be less specific about packages versions and remove it's dependencies.

>> But I don't know much about Python, so maybe Ed can make a call? :D
> 
> I'm not exactly an expert either :D

Still I learned useful things - really appreciate that!

BR,
M^2