Skip to content

fix crash in get_config_dict when copying modules that were imported in easyconfig file (like 'import os') #3729

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.

Sign up for GitHub

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
smoors merged 6 commits into from
Jun 10, 2021
Merged

fix crash in get_config_dict when copying modules that were imported in easyconfig file (like 'import os') #3729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8f95df
This generalizes the avoidance of attempting to pickle objects that
damianam Jun 7, 2021
4a206b3
Explicitely avoid __builtins__, otherwise the tests fail
damianam Jun 7, 2021
39d3d19
add test for parsing an easyconfig file that includes an import state…
boegel Jun 9, 2021
bbc6c8e
explicitly check for imported modules in get_config_dict rather than …
boegel Jun 9, 2021
558829b
avoid traceback in get_config_obj, report a clean error when copying …
boegel Jun 9, 2021
0012889
enhance test easyconfig in test_easyconfig_import to try to always tr…
boegel Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions easybuild/framework/easyconfig/format/one.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,14 @@ def get_config_dict(self):
# we can't use copy.deepcopy() directly because in Python 2 copying the (irrelevant) __builtins__ key fails...
cfg_copy = {}
for key in cfg:
if key != '__builtins__':
cfg_copy[key] = copy.deepcopy(cfg[key])
# skip special variables like __builtins__, and imported modules (like 'os')
if key != '__builtins__' and "'module'" not in str(type(cfg[key])):
Copy link
Contributor

@smoors smoors Jun 9, 2021

Choose a reason for hiding this comment

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

maybe also skip class objects?

... and "class 'type'" not in str(type(cfg[key])) and "type 'classobj'" not in str(type(cfg[key])):

Copy link
Member

@boegel boegel Jun 9, 2021

Choose a reason for hiding this comment

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

When would we ever have a class definition in easyconfigs though?!

Copy link
Contributor

@smoors smoors Jun 9, 2021

Choose a reason for hiding this comment

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

When something is possible, there will always be one person one day that will do it ;)

Copy link
Member

@boegel boegel Jun 9, 2021

Choose a reason for hiding this comment

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

Well sure, but there's all kinds of weird stuff you could in theory do in an easyconfig file, since it's essentially Python code. That doesn't mean we support all those things.

In 558829b I've made sure you don't get an ugly traceback with a funky easyconfig file, but a hopefully useful clean error message.

Copy link
Contributor

@smoors smoors Jun 9, 2021

Choose a reason for hiding this comment

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

that's perfectly fine, and I like that users will get a nice error message.

to be clear, we should indeed not support all possible funky stuff, but there's always a balance between what's useful to support and what can be implemented easily.
in this case the balance was not so clear-cut to me. some users prefer to put all logic into the easyconfig instead of the easyblock, so that could definitely be a useful use case (but again, I'm not saying we should support it)

Copy link
Member Author

@damianam damianam Jun 10, 2021

Choose a reason for hiding this comment

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

Just to add a little bit of context: We are starting to rely on import os to have "smart" easyconfigs that do different things depending on the system where they are being installed. I would pretty much like to keep this working in the future 😅 . There are of course other ways to do the same thing (hooks, easyblocks, overlays with slightly different EB files), but this is pretty convenient in some cases.

try:
cfg_copy[key] = copy.deepcopy(cfg[key])
except Exception as err:
raise EasyBuildError("Failed to copy '%s' easyconfig parameter: %s" % (key, err))
else:
self.log.debug("Not copying '%s' variable from parsed easyconfig", key)

return cfg_copy

Expand Down
37 changes: 37 additions & 0 deletions test/framework/easyconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4428,6 +4428,43 @@ def test_pure_ec(self):
self.assertEqual(ec_dict_bis.get('exts_default_options'), None)
self.assertEqual(ec_dict.get('sanity_check_paths'), {'dirs': ['bin'], 'files': [('bin/yot', 'bin/toy')]})

def test_easyconfig_import(self):
"""
Test parsing of an easyconfig file that includes import statements.
"""
test_ecs_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs', 'test_ecs')
toy_ec = os.path.join(test_ecs_dir, 't', 'toy', 'toy-0.0.eb')

test_ec = os.path.join(self.test_prefix, 'test.eb')
test_ec_txt = read_file(toy_ec)
test_ec_txt += '\n' + '\n'.join([
"import os",
"local_test = os.getenv('TEST_TOY')",
"sanity_check_commands = ['toy | grep %s' % local_test]",
])
write_file(test_ec, test_ec_txt)

os.environ['TEST_TOY'] = '123'

ec = EasyConfig(test_ec)

self.assertEqual(ec['sanity_check_commands'], ['toy | grep 123'])

# inject weird stuff, like a class definition that creates a logger instance
# and a local variable with a list of imported modules, to check clean error handling
test_ec_txt += '\n' + '\n'.join([
"import logging",
"class _TestClass(object):",
" def __init__(self):",
" self.log = logging.Logger('alogger')",
"local_test = _TestClass()",
"local_modules = [logging, os]",
])
write_file(test_ec, test_ec_txt)

error_pattern = r"Failed to copy '.*' easyconfig parameter"
self.assertErrorRegex(EasyBuildError, error_pattern, EasyConfig, test_ec)


def suite():
""" returns all the testcases in this module """
Expand Down