Skip to content

Update optional feature support of TensorFlow #2314

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

Merged

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 25, 2021

(created using eb --new-pr)

We used TF_NEED_* env variables to enable/disable features but those became out of sync with TF moving to --config options. This adds a check for all set TF_* variables that they are (still) relevant, i.e. contained in configure.py and prints a warning (@boegel IMO print_warning makes more sense here as a failure likely results in misconfiguration of TF, but you were more sparing with the before, so what shall it be?)
I also checked all variables through git bisect to find when they were introduced or removed and adapted the code to only use the valid ones.

There may be functional changes because --config=no* options were missing before but I'd consider those as a bugfix, not a change because TF_NEED_*=0 was used for those.

@Flamefire Flamefire force-pushed the 20210125121255_new_pr_yfsAnCxQAQ branch from 474173f to b3e5609 Compare February 12, 2021 13:33
@Flamefire
Copy link
Contributor Author

Rebased and dependencies removed. No changes and this was already tested with easybuilders/easybuild-easyconfigs#12037, so should be good to go

@boegel
Copy link
Member

boegel commented Feb 12, 2021

@boegel IMO print_warning makes more sense here as a failure likely results in misconfiguration of TF, but you were more sparing with the before, so what shall it be?

Since the impact of defining $TF_* environment variables that have no impact is very limiting, log.warning rather than print_warning makes sense.

We should only use print_warning when the eb end user really needs to be made aware of something, not when an easyblock maintainer needs to double check something.

Co-authored-by: Kenneth Hoste <[email protected]>
@boegel
Copy link
Member

boegel commented Feb 12, 2021

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS TensorFlow-1.15.5-fosscuda-2019b-Python-3.7.4.eb
  • SUCCESS TensorFlow-2.3.1-fosscuda-2019b-Python-3.7.4.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3307.joltik.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6242 CPU @ 2.80GHz (cascadelake), Python 3.6.8
See https://gist.github.com/621da539285a380183ce28fcf040be63 for a full test report.

@boegel
Copy link
Member

boegel commented Feb 12, 2021

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS TensorFlow-1.15.2-foss-2019b-Python-3.7.4.eb
  • SUCCESS TensorFlow-2.1.0-fosscuda-2019b-Python-3.7.4.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node2715.swalot.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz (haswell), Python 3.6.8
See https://gist.github.com/c5b293228964c6ad2c52b0ce2aff3db9 for a full test report.

@Flamefire Flamefire deleted the 20210125121255_new_pr_yfsAnCxQAQ branch February 13, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants