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

[luigi.contrib.external_program.ExternalProgramTask] logs_output_pattern_to_url provided #2822

Merged
merged 5 commits into from Dec 3, 2019

Conversation

drowoseque
Copy link
Contributor

Description

Method logs_output_pattern_to_url added to ExternalProgramTask

Motivation and Context

Sometimes our external tasks do not contain url in output logs but one easily can be restored from output
For example if we run Tez Task which outputs YARN application id only
we can restore tracking url by appending a resource manager address + endpoint
That could be done by using logs_output_pattern_to_url

Have you tested this? If so, how?

I've provided unit tests for that

@drowoseque
Copy link
Contributor Author

@honnix @Tarrasch @dlstadther could you please review this PR?

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

I don't see any functional change here. Please explain.

luigi/contrib/external_program.py Outdated Show resolved Hide resolved
@honnix
Copy link
Contributor

honnix commented Dec 3, 2019

LGTM now.

@drowoseque
Copy link
Contributor Author

@honnix
okay, can we go on with that?

@honnix honnix merged commit 63bb867 into spotify:master Dec 3, 2019
@honnix
Copy link
Contributor

honnix commented Dec 4, 2019

@drowoseque Could you please take a look why this broke master CI? Thanks.

@drowoseque
Copy link
Contributor Author

I see similar failures in https://travis-ci.org/spotify/luigi/jobs/618083277 but in next build this test was okay https://travis-ci.org/spotify/luigi/builds/619552243
It seems that worker_test.DynamicDependenciesWithMultipleWorkersTes.test_dynamic_dependenciest is flaky

======================================================================
FAIL: test_dynamic_dependencies (worker_test.DynamicDependenciesWithMultipleWorkersTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/spotify/luigi/test/worker_test.py", line 1066, in test_dynamic_dependencies
    self.assertTrue(time.time() - t0 < self.timeout)
AssertionError: False is not true

@drowoseque
Copy link
Contributor Author

@honnix could you please rerun ci ?

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.

None yet

3 participants