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

Add jinja2 functions env and versionformat #83

Closed

Conversation

allanleal
Copy link
Contributor

This PR proposes the addition of two functions in the jinja2 dictionary of variables. They are env and versionformat.

  • The function env is just a syntactic sugar to os.environt.get method.
  • The function versionformat is useful when version information is available in integer format, but a semantic version format is needed (e.g., 37 instead of 3.7).

With the proposed changes, the following conda-devenv yaml file:

{% set conda_py = os.environ.get('CONDA_PY', '35') %}

name: web-ui-py{{ conda_py }}

dependencies:
  - python={{ '.'.join(conda_py) }}
  - boost
  - cmake
  - gcc     # [linux]
  - ccache  # [not win]
  - clcache # [win]

becomes:

{% set conda_py = env('CONDA_PY') or '35' %}

name: web-ui-py{{ conda_py }}

dependencies:
  - python={{ versionformat(conda_py) }}
  - boost
  - cmake
  - gcc        # [linux]
  - ccache     # [not win]
  - clcache    # [win]

@edisongustavo
Copy link
Contributor

I will give my honest feedback on these 2 functions:

env

I don't see why typing os.environ['VAR_NAME'] is such a hassle. If you really want the alias, you could do in your Jinja file in the beginning:

{% set env = os.environ.env %}

I believe this would be enough.

My take is that adding env creates "surprise", making conda devenv less interchangeable with conda env, which is a bad thing in my opinion.

Sometimes less is more.

versionformat

I understand where you're coming from with that function, but I think it is not really well defined.

What should it return for other numbers, like 1131? Which of:

  • 11.3.1
  • 1.1.3.1
  • 1.13.1

I think you can understand my point.

@allanleal
Copy link
Contributor Author

Thanks for your opinion.

Regarding the use of:

{% set env = os.environ.env %}

os.environ.env does not exist. Maybe {% set env = os.environ %} should work but this gives access to the entire dictionary and env['VAR'] will result in error if 'VAR' is not there. OK, one could use env.get('VAR') to have None in this case.

As to versionformat, it intrinsically assumes the same limitations as the already existing ones for the integer format adopted for CONDA_PY in conda-build. If one day Python goes to version 3.11, how is CONDA_PY, with a value '311', be interpreted? 3.1.1 or 3.11? The implementation logic versionformat would follow the same convention.


Why the above two methods should be included in conda-devenv?

If one uses conda-devenv in multiple projects, and the following needs are always recurring:

  1. environment variables are always needed in environment.devenv.yml files; and
  2. CONDA_PY is always used to resolve the version of python,

I think these two utility methods would be extremely beneficial, simplify the environment.devenv.yml files, avoid duplication of auxiliary jinja/python codes and eliminate cryptic codes such as '.'.join(os.environ.get('CONDA_PY', '37')).

If the idea that none of these small, but useful, additions are needed in conda-devenv persists, I'll happily close this PR - no worries!

@marcelotrevisani
Copy link
Contributor

I think would be better if conda-devenv follows the predefined selector in conda and/or the build var env

You can use the pre-selectors such PY_VER, PY3K, PY36 and so on....

I agree with @edisongustavo , I think that might be an error-prone implementation

@allanleal
Copy link
Contributor Author

The env method is not critical, I agree.

The versionformat is also not needed if conda-devenv automatically creates an environment variable called PY_VER with the python version (e.g., if CONDA_PY is 36, PY_VER is 3.6, just like conda-build does).

This would keep conda-devenv usage more or less consistent with that of conda-build.

So, what about instead of env and versionformat, we have, in this PR, the addition of PY_VER as an environment variable created once the conda env is activated? This is more in line with the suggestion of @marcelotrevisani .

@marcelotrevisani
Copy link
Contributor

I think that should be better! @allanleal

@prusse-martin
Copy link
Member

@marcelotrevisani
About selectors.
I don't see python version or anything about "the to be created environment" selectors beeing implemented without caling the conda solver before hand and for that the selectors should already be processed what lead us to a deadlock.

@marcelotrevisani
Copy link
Contributor

Oh sorry, @prusse-martin , I think I was not clear.
I meant to follow at least the same nomenclature which you can find in the variables in conda environment.
But not to use them, because (as you said) that is not possible.

@allanleal
Copy link
Contributor Author

allanleal commented Feb 20, 2019

@prusse-martin Thanks for the observation about the selectors. Does this also apply to the env var PY_VER? One reads here the following:

PY_VER  Python version building against. Set with the python argument or with the CONDA_PY environment variable.

Based on an existing CONDA_PY, we would need some code in the activation method to set PY_VER accordingly. Is this possible?

@nicoddemus
Copy link
Member

My 2 cents:

I'm not against having env as an alias to os.environ.get, but I feel this might steer us in the direction of having special aliases for everything, which I'm not sure is a good idea. Sticking to only modules feels safer in the long term.

About formatversion, I agree with @edisongustavo's points about this not being very well defined. While @allanleal's points about it sticking to whatever CONDA_PY means, CONDA_PY itself is not really a strong standard either, just something we kind of borrowed from conda-build and which is not even used there anymore IIRC. So I prefer to leave optioned functions that muck with versions out for now.

My two points lean heavily on the mantra of "it is easy to put something as a feature or part of an API, but very hard to get it out" because then you need to worry about backward compatibility. So I lean on being very careful to adding new features in general, specially when they are a little murky.

@prusse-martin
Copy link
Member

@allanleal If you plan to have that available during parsing the jinja2 code yes it does applies.
If you are saying we should make it available once the environment is activated it is possible.
But it will incur some extra processing and the are 2 options:

  1. Add in the activate scripts some code the detect the python version durring activation time and set the env var (no overhead in the devenv call but overhead in each activation);
    or
  2. When rendering the activate/deactivate scripts obtain the linked python version to render that in the scripts (overhead in the devenv call and no noticeable overhead in activation calls);

But do you have a use case where is that necessary?

@allanleal
Copy link
Contributor Author

@nicoddemus I agree with all you just said. Having said that, yes, let's not have these functions.

The ongoing discussion now is if we should have, for example, the environment variable PY_VER set once we execute conda activate <project>. I see this is available in conda-build, but maybe it should as well not be added to conda devenv.

My main interest is to have a simple way to reference the python version of the conda environment in the environment.devenv.yml files when CONDA_PY is used as a controlling build variable (e.g., multiples builds with different CONDA_PY values).

Maybe you guys are using an alternative solution that works well and this addition to conda-devenv is too not needed.

@allanleal
Copy link
Contributor Author

allanleal commented Feb 20, 2019

@prusse-martin When we use CONDA_PY to control builds. For example:

  matrix:
  - CONFIG: Debug
    CONDA_PY: 36
  - CONFIG: Release
    CONDA_PY: 37

@prusse-martin
Copy link
Member

prusse-martin commented Feb 20, 2019

If the env var CONDA_PY is set in before environment activation it will be available once the environment is actived also.

@allanleal
Copy link
Contributor Author

allanleal commented Feb 20, 2019

@prusse-martin

If the env var CONDA_PY is set in before environment activation it will be available once the environment is actived also.

Yes. During activation, PY_VER would be set accordingly (if CONDA_PY is 'XY', then PY_VER is 'X.Y').

@nicoddemus
Copy link
Member

My main interest is to have a simple way to reference the python version of the conda environment in the environment.devenv.yml files when CONDA_PY is used as a controlling build variable (e.g., multiples builds with different CONDA_PY values).

For that case you can use the version directly then:

  matrix:
  - CONFIG: Debug
    CONDA_PY: 3.6
  - CONFIG: Release
    CONDA_PY: 3.7

Then in your devenv.yml:

dependencies:
    - python={{ os.environ.get('CONDA_PY', '3.6')

Or even call it something else (PYTHON_VER) to avoid confusion with CONDA_PY, which we borrowed some time ago from conda-build but is no longer even used.

Yes. During activation, PY_VER would be set accordingly (if CONDA_PY is XY, then PY_VER is 'X.Y`).

This seems confusing, why not use a single environment variable for everything? Perhaps you are under the impression that CONDA_PY is a requirement? It is not, it is just some convention we adopted early on when we started using conda.

@allanleal
Copy link
Contributor Author

@nicoddemus

@tadeu and I had a discussion about this in the past, in using CONDA_PY exactly as you just wrote, e.g., 3.6. See here reaktoro/reaktoro#41 (comment)

The idea would be to use CONDA_PY with the same convention as other conda tools.

@nicoddemus
Copy link
Member

The idea would be to use CONDA_PY with the same convention as other conda tools.

Oh @tadeu might comment here then... I thought only conda-build used CONDA_PY, and not even so anymore after conda-build 3 where it now accepts a --python flag:

--python PYTHON       Set the Python version used by conda build.

@allanleal
Copy link
Contributor Author

Since conda-devenv does not interfere with conda-build (because this one requires its own recipe), maybe I should replace CONDA_PY in my use cases by something else (say, PYTHON_VERSION) and set this to 2.7 or 3.6, as you suggest.

Thus, we forget about this need of PY_VER initialized based on CONDA_PY before activation. env is not far away from os.environ.get and versionformat is no longer needed because no more nonsense of 27, 36, and 37 as version numbers.

Meaning, we close this PR! 😉 🎉

@nicoddemus
Copy link
Member

Great, thanks! I think the the whole discussion was really worthwhile anyway! 👍

Thanks everyone who participated.

@nicoddemus nicoddemus closed this Feb 20, 2019
@edisongustavo
Copy link
Contributor

edisongustavo commented Feb 20, 2019

I think something that we have to remember on this project: conda-devenv is an "enhanced version" of conda-env. Not conda-build.

The addition of selectors from conda-build is nice, but then we're entering muddy waters, so we have to be very careful when dealing in this territory.

Or am I mistaken?

@edisongustavo
Copy link
Contributor

Also, something that I'm thinking is that we should probably add a way for users to add their "custom functions" when using conda-devenv without having to change the source code of conda-devenv.

If we add this feature then his versionformat() and env() functions would be available on his own usage of conda-devenv.

What do you guys think? If you agree I will create an issue with more details.

@allanleal
Copy link
Contributor Author

I think you're absolutely right @edisongustavo . I would just say that good ideas adopted there (and possibly in other tools) might be suitable here too. For example, preprocessing selectors in comments such as # [linux] is very handy.

@allanleal
Copy link
Contributor Author

allanleal commented Feb 20, 2019

we should probably add a way for users to add their "custom functions"

This would be great, @edisongustavo . I tried before doing this:

{% set versionformat = lambda ver: '.'.join(ver) %}

and it did not work. If you could add this support, or something similar, that would be appreciated.

@prusse-martin
Copy link
Member

@edisongustavo are you suggesting some thing like the symbols in a file akin to .conda-devenv.py to the added to the jinja context when rendering?

@nicoddemus
Copy link
Member

I think we should open another issue to discuss this. 😁

@edisongustavo
Copy link
Contributor

@edisongustavo are you suggesting some thing like the symbols in a file akin to .conda-devenv.py to the added to the jinja context when rendering?

Yes. That would be awesome.

@edisongustavo
Copy link
Contributor

I think we should open another issue to discuss this.

Agree.

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.

None yet

5 participants