Fix env type

[andre-merzky]

This addresses #314: job's environment dict can now have string and int types - int types are cast to string types.

[hategan]

I need to undraft this for a minute to test something.

[andre-merzky]

@hategan : can I have your advice on the mypy error? I seem to be somewhat blind on why the environment.setter declaration does not kick in :-(

[hategan]

@hategan : can I have your advice on the mypy error? I seem to be somewhat blind on why the environment.setter declaration does not kick in :-(

Could it be https://mypy.readthedocs.io/en/stable/common_issues.html#variance?

I.e., you cannot assign a Dict[str, str] to a Dict[str, str | int]. But Mapping is covariant, so a Mapping[str, str] can be assigned to a Mapping[str, str | int].

[andre-merzky]

@hategan : can I have your advice on the mypy error? I seem to be somewhat blind on why the environment.setter declaration does not kick in :-(

Could it be https://mypy.readthedocs.io/en/stable/common_issues.html#variance?

I.e., you cannot assign a Dict[str, str] to a Dict[str, str | int]. But Mapping is covariant, so a Mapping[str, str] can be assigned to a Mapping[str, str | int].

Thanks @hategan , that looks like a good read!

[andre-merzky]

Thanks for the pointer to Mapping, that does not seem to do the trick either though - I don't get mypy to like this without too much code to be worth the trouble really. I see two options: silence the mypy warning (ugh), or drop the idea of accepting int values. At this point I am fine with the second option also. Let me know what you think :-)

[hategan]

My apologies. I think we are hitting an entirely different issue than the plain invariance that I quoted.

What seems to be happening, in this particular case, is that mypy takes the type of the property from the getter. I say "in this particular case" because the same thing is not happening with all the Path | str getters/setters. It would appear that mypy should support this (see python/mypy#3004), but it is unclear to my why it breaks for the dict and not paths.

[hategan]

but it is unclear to my why it breaks for the dict and not paths.

It also breaks for paths. It's just that we haven't tested it. For me, spec.directory = '/tmp' results in a mypy error.

[hategan]

see python/mypy#3004

That issue was opened in 2017. I had incorrectly assumed that it was fixed not long after. However, it is still open.

[hategan]

I see two options: silence the mypy warning (ugh), or drop the idea of accepting int values. At this point I am fine with the second option also. Let me know what you think :-)

I slept on it and I think we should allow int values. The worst case scenario is that mypy does not get fixed. In that case, even with this patch, we do not lose any functionality because all code that treats spec.environment as a Dict[str, str] will continue to function correctly and pass mypy tests. External code that passes ints as env values and does not check things with mypy will benefit from the addition. External code that uses mypy has the option of disabling checks on those specific assignments. So essentially there is still a gain with virtually no loss.

I think the only issue is that we'll need to address the mypy error in the constructor. For that, we can do what the Path|str properties do: refactor the conversion into a function and call that function in both the constructor and the setter.

[andre-merzky]

I slept on it and I think we should allow int values.

If you are ok with that approach I'm happy to go that route also. I certainly agree on the pros and cons. Thanks!

[hategan]

I slept on it and I think we should allow int values.

If you are ok with that approach I'm happy to go that route also. I certainly agree on the pros and cons. Thanks!

I think my vote is to be OK with said approach.

[hategan]

Looks good, but I'm not quite sure about the test executor template change.

[hategan]

I would caution against assuming that /bin is in PATH or that PATH is used when launching executables.

[hategan]

I' m trying to understand this change. It was:

export --ignore-environment \
    A='a' \
    b='b' \
    command

That is, env is used to set the variables and run the command.

It looks like now it will expand to

export --ignore-environment \
    export A='a'
    export B='b'
    command

Here env is used to set the first variable, although I think this should fail since env will try to run the export executable, which is a bash builtin, so it will fail. Even if not, it will only set the first variable. Then the second and subsequent variables will be set, and the command run with the inherited environment (since the scope of the effects of env --ignore-environment has ended) and all but the first variable set.

[andre-merzky]

jeah, my 'fix' was not a fix apparently. The problem is that I don't get the

export --ignore-environment \
    A='a' \
    b='b' \
    command

but rather see

export --ignore-environment \
    A='a' \
    
    b='b' \
    command

(See mustache.txt for example). I tried to get rid of the newline, but the escaping is a bit funny with mustache :-) Reverted now, please let me know if you have any advise.

[hategan]

Indeed, and we weren't testing for multiple env vars.

I think the issue is that there is a newline after {{#env}}, which puts a newline before each env var line, and there is one after each env var, which adds another.

The following seems to work for me:

{{#job.spec.inherit_environment}}env \{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}}
{{name}}="{{value}}" \{{/env}}
{{#psij.launch_command}}{{.}} {{/psij.launch_command}}

I.e., only add newline before each env var and before the launch command.

[hategan]

Perhaps it would be wise to fix in all executors, but I don't want to hold this up if that's too much effort.

[andre-merzky]

I don't think the other executors suffer from the same problem as they create individual export directives per env pair?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c07a62d) 74.60% compared to head (88bda65) 74.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   74.60%   74.69%   +0.09%     
==========================================
  Files          94       94              
  Lines        3890     3904      +14     
==========================================
+ Hits         2902     2916      +14     
  Misses        988      988              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hategan]

Thanks Andre.