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

Fix serialization of tasks with environment variables set #1066

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

jeevb
Copy link
Contributor

@jeevb jeevb commented Jun 16, 2022

TL;DR

Fixes serialization of tasks with environment variables set.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Fixes PythonAutoContainer.get_container to handle the case where SerializationSettings.env is None (See:

env: Optional[Dict[str, str]] = None
). Currently fails with:

Traceback (most recent call last):
  File "/fn/bin/pyflyte", line 8, in <module>
    sys.exit(main())
  File "/fn/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/fn/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/fn/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/fn/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/fn/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/fn/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/fn/lib/python3.9/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/fn/lib/python3.9/site-packages/flytekit/clis/sdk_in_container/serialize.py", line 139, in workflows
    serialize_all(
  File "/fn/lib/python3.9/site-packages/flytekit/exceptions/scopes.py", line 160, in system_entry_point
    return wrapped(*args, **kwargs)
  File "/fn/lib/python3.9/site-packages/flytekit/clis/sdk_in_container/serialize.py", line 69, in serialize_all
    serialize_to_folder(pkgs, serialization_settings, local_source_root, folder)
  File "/fn/lib/python3.9/site-packages/flytekit/tools/repo.py", line 61, in serialize_to_folder
    loaded_entities = serialize(pkgs, settings, local_source_root, options=options)
  File "/fn/lib/python3.9/site-packages/flytekit/tools/repo.py", line 46, in serialize
    registrable_entities = get_registrable_entities(ctx, options=options)
  File "/fn/lib/python3.9/site-packages/flytekit/tools/serialize_helpers.py", line 77, in get_registrable_entities
    get_serializable(new_api_serializable_entities, ctx.serialization_settings, entity, options=options)
  File "/fn/lib/python3.9/site-packages/flytekit/tools/translator.py", line 573, in get_serializable
    cp_entity = get_serializable_task(entity_mapping, settings, entity)
  File "/fn/lib/python3.9/site-packages/flytekit/tools/translator.py", line 173, in get_serializable_task
    container = entity.get_container(settings)
  File "/fn/lib/python3.9/site-packages/flytekit/core/python_auto_container.py", line 159, in get_container
    env = {**settings.env, **self.environment} if self.environment else settings.env
TypeError: 'NoneType' object is not a mapping

assert c.env == {"FOO": "bar", "HAM": "spam"}


def test_get_container_without_serialization_settings_envvars(minimal_serialization_settings):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails without the code change.

@jeevb jeevb marked this pull request as ready for review June 16, 2022 16:21
@jeevb
Copy link
Contributor Author

jeevb commented Jun 16, 2022

@ggydush-fn: FYI.

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.53%. Comparing base (87d1390) to head (098d13c).
Report is 1055 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage   86.52%   86.53%   +0.01%     
==========================================
  Files         268      268              
  Lines       24877    24896      +19     
  Branches     2802     2804       +2     
==========================================
+ Hits        21525    21544      +19     
  Misses       2874     2874              
  Partials      478      478              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

wow, great catch!

@eapolinario eapolinario merged commit ffd942c into master Jun 16, 2022
@jeevb jeevb deleted the jeev/fix-serialization-with-envvars branch June 16, 2022 18:39
eapolinario pushed a commit that referenced this pull request Jun 17, 2022
@eapolinario eapolinario mentioned this pull request Jun 17, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants