-
Notifications
You must be signed in to change notification settings - Fork 217
A couple of bug fixes with tweak.py for --try-update-deps
#3325
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tweak.py for --try-update-deps
Contributor
Author
|
Requesting comment from @ocaisa |
ocaisa
reviewed
May 6, 2020
Contributor
Author
|
I think relying on parsing the file name is somewhat brittle. Version numbers can contain dashes, version suffixes can also contain dashes I think, software name can contain dashes. I’m not sure if anything would prevent a tool chain name from containing dashes as well.
It did not give any noticeable slow down. Doing the easybuild bits is not what is usually slow when building a package. I would not recommend over optimizing this.
…
On May 6, 2020 at 3:14 AM, <ocaisa ***@***.***)> wrote:
@ocaisa commented on this pull request.
In easybuild/framework/easyconfig/tweak.py (#3325 (comment)):
> @@ -1089,6 +1090,9 @@ def find_potential_version_mappings(dep, toolchain_mapping, versionsuffix_mappin # Figure out the main versionsuffix (altered depending on toolchain in the loop below) versionsuffix = dep.get('versionsuffix', '') + # If versionsuffix is equal to None, it should be put to empty string + if not versionsuffix:
Shouldn't we explicitly check for is None here?
In easybuild/framework/easyconfig/tweak.py (#3325 (comment)):
> @@ -1129,6 +1131,21 @@ def find_potential_version_mappings(dep, toolchain_mapping, versionsuffix_mappin tweaked_ecs_paths, _ = alt_easyconfig_paths(tempfile.gettempdir(), tweaked_ecs=True) cand_paths = [path for path in cand_paths if not path.startswith(tweaked_ecs_paths)] + # if SYSTEM_TOOLCHAIN_NAME is used, it produces regex of the form + # <name>-<version_regex>.eb, which can map to incompatible toolchains. + # For example Boost-1.68\..*.eb would match Boost-1.68.0-intel-2019a.eb + # This filters out such matches unless the toolchain in the easyconfig matches a system toolchain + if toolchain['name'] == SYSTEM_TOOLCHAIN_NAME: + cand_paths_filtered = [] + for path in cand_paths: + tc_candidate = fetch_parameters_from_easyconfig(read_file(path), ['toolchain'])[0]
Because of the search pattern being greedy this could be pretty slow because we will potentially be reading from a lot of files multiple times. I guess the matching is pretty limiting though, did this give a noticeable slowdown?
The only condition for filtering I can think of that would avoid having to read the files is that the greedy matching cannot contain 2 - which would/could indicate the presence of a toolchain (-intel-2019). I'm no regex expert though but I will give it a shot.
In easybuild/framework/easyconfig/tweak.py (#3325 (comment)):
> @@ -1129,6 +1131,21 @@ def find_potential_version_mappings(dep, toolchain_mapping, versionsuffix_mappin tweaked_ecs_paths, _ = alt_easyconfig_paths(tempfile.gettempdir(), tweaked_ecs=True) cand_paths = [path for path in cand_paths if not path.startswith(tweaked_ecs_paths)] + # if SYSTEM_TOOLCHAIN_NAME is used, it produces regex of the form + # <name>-<version_regex>.eb, which can map to incompatible toolchains. + # For example Boost-1.68\..*.eb would match Boost-1.68.0-intel-2019a.eb + # This filters out such matches unless the toolchain in the easyconfig matches a system toolchain + if toolchain['name'] == SYSTEM_TOOLCHAIN_NAME: + cand_paths_filtered = [] + for path in cand_paths: + tc_candidate = fetch_parameters_from_easyconfig(read_file(path), ['toolchain'])[0]
How about:
filename = os.path.basename(path) version_match = re.search("(?<=" + prefix_to_version + ").*?(?=\.eb)", txt) # Heuristic to check if there's a toolchain hidden in the match non_system_toolchain = version_match.group(0).count('-') >= 2 if not non_system_toolchain : cand_paths_filtered += [path]
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#3325 (review)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/ABZKY2T6X2KX5NDQLNSRCBDRQEE6VANCNFSM4MZ2AOGA).
|
ocaisa
reviewed
May 6, 2020
…ffix to None explicitly
ocaisa
approved these changes
May 6, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For whatever reason, one of our dependencies has the following form (once converted to string) :
{'full_mod_name': 'Core/scipy-stack/2019a', 'short_mod_name': 'scipy-stack/2019a', 'name': 'SciPy-Stack', 'version': '2019a', 'versionsuffix': None, 'toolchain': {'name': 'system', 'version': ''}, 'toolchain_inherited': False, 'system': True, 'hidden': False, 'build_only': True, 'external_module': False, 'external_module_metadata': {}}i.e.
versionsuffixis explicitly set toNone.This causes an error when concatenating to get
full_versionsuffix: