Skip to content

include --hidden in job script when needed#1356

Merged
boegel merged 4 commits intoeasybuilders:developfrom
boegel:fix_job_hidden_external
Aug 14, 2015
Merged

include --hidden in job script when needed#1356
boegel merged 4 commits intoeasybuilders:developfrom
boegel:fix_job_hidden_external

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Aug 14, 2015

fix for #1328

This also fixes an other issue I ran into, i.e. dependencies marked as external modules were not being filtered when determining job dependencies, which manifested itself as:

======================================================================
ERROR: test_build_easyconfigs_in_parallel_pbs_python (__main__.ParallelBuildTest)
Test build_easyconfigs_in_parallel(), using (mocked) pbs_python as backend for --job.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/framework/parallelbuild.py", line 121, in test_build_easyconfigs_in_parallel_pbs_python
    jobs = build_easyconfigs_in_parallel("echo '%(spec)s'", ordered_ecs, prepare_first=False)
  File "/Users/kehoste/work/easybuild-framework/easybuild/tools/parallelbuild.py", line 97, in build_easyconfigs_in_parallel
    new_job = create_job(active_job_backend, build_command, ec, output_dir=output_dir)
  File "/Users/kehoste/work/easybuild-framework/easybuild/tools/parallelbuild.py", line 174, in create_job
    if easyconfig['ec']['hidden']:
  File "/Users/kehoste/work/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 102, in new_ec_method
    return ec_method(self, key, *args, **kwargs)
  File "/Users/kehoste/work/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 694, in __getitem__
    raise EasyBuildError("Use of unknown easyconfig parameter '%s' when getting parameter value", key)
EasyBuildError: "Use of unknown easyconfig parameter 'hidden' when getting parameter value"

----------------------------------------------------------------------
Ran 2 tests in 3.140s

cc @ocaisa

This was referenced Aug 14, 2015
Merged
Closed
@boegel boegel added this to the 2.2.1 milestone Aug 14, 2015
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Aug 14, 2015

@wpoely86, @JensTimmerman, @rjeschmi: please review?

@hpcugentbot
Copy link
Copy Markdown

hpcugentbot commented Aug 14, 2015

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1969/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1969/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

wpoely86
wpoely86 reviewed Aug 14, 2015
View reviewed changes
Comment thread easybuild/tools/parallelbuild.py Outdated
Copy link
Copy Markdown
Member

@wpoely86 wpoely86 Aug 14, 2015

Choose a reason for hiding this comment

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

why the unset TMPDIR?

Copy link
Copy Markdown
Member

@wpoely86 wpoely86 Aug 14, 2015

Choose a reason for hiding this comment

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

missing space between add_opts and --testoutput?

Copy link
Copy Markdown
Member Author

@boegel boegel Aug 14, 2015

Choose a reason for hiding this comment

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

I'm not sure the unset $TMPDIR && is still needed (since eb set it itself), but it was at some point...

Tried to figure it out with git log -L136,136:easybuild/tools/parallelbuild.py, but no luck, it's been there since forever.

Probably since before EB starting setting $TMPDIR and taking control over what directory tempfile.* is using, so it's likely it can go. But I'm not going to do that in this PR. :-)

@wpoely86
Copy link
Copy Markdown
Member

wpoely86 commented Aug 14, 2015

looks fine

@hpcugentbot
Copy link
Copy Markdown

hpcugentbot commented Aug 14, 2015

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1972/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1972/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Aug 14, 2015

Thanks for the review @wpoely86!

boegel added a commit that referenced this pull request Aug 14, 2015
@boegel
Merge pull request #1356 from boegel/fix_job_hidden_external
5643657 
include --hidden in job script when needed
@boegel boegel merged commit 5643657 into easybuilders:develop Aug 14, 2015
@boegel boegel deleted the fix_job_hidden_external branch August 14, 2015 21:55
@boegel boegel mentioned this pull request Aug 15, 2015
Merged
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Aug 15, 2015

I've should've known better than to merge this without having a unit test for dep_graph, since switching away from unresolved_deps broke --dep-graph (it resulted in an unconnected set of nodes being dumped, rather than an actual graph).

#1359 fixes the issue, and adds a unit test for dep_graph

@boegel boegel mentioned this pull request Aug 16, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@boegel @hpcugentbot @wpoely86