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 options to run unit tests to TensorFlow EasyBlock #2263

Merged

Conversation

Flamefire
Copy link
Contributor

(created using eb --new-pr)

Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the mystery removal this lgtm

@@ -636,9 +640,6 @@ def configure_step(self):
self.install_base_dir))]
apply_regex_substitutions('configure.py', regex_subs)

# Tell Bazel to not use $HOME/.cache/bazel at all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed without justification and looks unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm currently testing why the tests still fail (they are run at least) and found that we add to many options. Only thing needed seems to be --output_user_root, TEST_TMPDIR is set internally by Bazel to tell Bazel that a test is being run. Setting that for compilation is hence wrong. I'll double check if output_base and install_base can also be removed and would include that here, or shall I make a second PR where those options are removed? Problem then would be merge conflicts which I'd want to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A second PR would be best indeed.
Perhaps a rebase can avoid merge conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see. I undid this and if this is merged first I can keep the other changes and likely rebase them

@boegel boegel added this to the 4.4.0 milestone Dec 8, 2020
@Flamefire
Copy link
Contributor Author

@bartoldeman ping

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akesandgren akesandgren dismissed bartoldeman’s stale review December 18, 2020 15:39

Requested changed done

@akesandgren
Copy link
Contributor

Going in, thanks @Flamefire!

@akesandgren akesandgren merged commit b54481b into easybuilders:develop Dec 18, 2020
@Flamefire Flamefire deleted the 20201203092107_new_pr_epgQeAsthx branch December 18, 2020 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants