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

[23.1] Fix and prevent persisting null file_size #16855

Merged
merged 6 commits into from
Oct 16, 2023

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Oct 15, 2023

This can happen in extended metadata mode.
Fixes:

TypeError: float() argument must be a string or a number, not 'NoneType'
  File "galaxy/jobs/mapper.py", line 258, in __cache_job_destination
    self.cached_job_destination = self.__determine_job_destination(
  File "galaxy/jobs/mapper.py", line 242, in __determine_job_destination
    job_destination = self.__handle_dynamic_job_destination(raw_job_destination)
  File "galaxy/jobs/mapper.py", line 205, in __handle_dynamic_job_destination
    return self.__handle_rule(expand_function, destination)
  File "galaxy/jobs/mapper.py", line 220, in __handle_rule
    raise e
  File "galaxy/jobs/mapper.py", line 209, in __handle_rule
    job_destination = self.__invoke_expand_function(rule_function, destination)
  File "galaxy/jobs/mapper.py", line 125, in __invoke_expand_function
    return expand_function(**actual_args)
  File "tpv/rules/gateway.py", line 51, in map_tool_to_destination
    return ACTIVE_DESTINATION_MAPPER.map_to_destination(app, tool, user, job, job_wrapper, resource_params,
  File "tpv/core/mapper.py", line 168, in map_to_destination
    evaluated_entity = self.match_combine_evaluate_entities(context, tool, user)
  File "tpv/core/mapper.py", line 138, in match_combine_evaluate_entities
    evaluated_entity = combined_entity.evaluate_rules(context)
  File "tpv/core/entities.py", line 488, in evaluate_rules
    if rule.is_matching(context):
  File "tpv/core/entities.py", line 742, in is_matching
    if self.loader.eval_code_block(self.match, context):
  File "tpv/core/loader.py", line 49, in eval_code_block
    'input_size': helpers.input_size(context['job']) if 'input_size' in str(code) else 0
  File "tpv/core/helpers.py", line 32, in input_size
    return calculate_dataset_total(job.input_datasets)/GIGABYTES
  File "tpv/core/helpers.py", line 26, in calculate_dataset_total
    return reduce(sum_total, map(get_dataset_size, unique_datasets.values()), 0.0)
  File "tpv/core/helpers.py", line 16, in get_dataset_size
    return float(dataset.file_size)

from https://sentry.galaxyproject.org/share/issue/96123a088f554a8198b300989af27818/

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.2 milestone Oct 15, 2023
@mvdbeek mvdbeek force-pushed the file_size_debug branch 2 times, most recently from 9443cbd to 316a20b Compare October 15, 2023 12:43
@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 15, 2023

Turns out we were persisting a null file_size for terminal datasets all over the place, like when failing a dataset or just accidentally while committing other things while the dataset / job was still being handled, not just in the extended metadata case. Inspecting the dirty object in a session is really useful for finding all of these (as well as unnecessary flushes).

Copy link
Member

@natefoo natefoo left a comment

Choose a reason for hiding this comment

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

🎉

@mvdbeek mvdbeek merged commit d01a21a into galaxyproject:release_23.1 Oct 16, 2023
38 checks passed
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.

2 participants