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 default argument check on py> operator #913

Merged
merged 1 commit into from
Jan 27, 2019
Merged

Fix default argument check on py> operator #913

merged 1 commit into from
Jan 27, 2019

Conversation

chezou
Copy link
Member

@chezou chezou commented Oct 31, 2018

Fix #912

Checked with a.dig and a.py described in #912

Before

➜  test-py3 digdag run a.dig
2018-10-31 17:36:09 +0900: Digdag v0.9.30
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$2 (file:/Users/ariga/bin/digdag) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$2
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
2018-10-31 17:36:10 +0900 [WARN] (main): Using a new session time 2018-10-31T00:00:00+00:00.
2018-10-31 17:36:10 +0900 [INFO] (main): Using session /Users/ariga/src/test-py3/.digdag/status/20181031T000000+0000.
2018-10-31 17:36:10 +0900 [INFO] (main): Starting a new session project id=1 workflow name=a session_time=2018-10-31T00:00:00+00:00
2018-10-31 17:36:17 +0900 [INFO] (0019@[0:default]+a+t1): py>: a.ok
(111, 2, 3)
2018-10-31 17:36:23 +0900 [INFO] (0019@[0:default]+a+t2): py>: a.ng
Traceback (most recent call last):
  File "<stdin>", line 147, in <module>
  File "<stdin>", line 129, in digdag_inspect_arguments
TypeError: Method 'ng' requires parameter 'arg3' but not set
2018-10-31 17:36:23 +0900 [ERROR] (0019@[0:default]+a+t2): Task failed with unexpected error: Python command failed with code 1
java.lang.RuntimeException: Python command failed with code 1
	at io.digdag.standards.operator.PyOperatorFactory$PyOperator.runCode(PyOperatorFactory.java:153)
	at io.digdag.standards.operator.PyOperatorFactory$PyOperator.runTask(PyOperatorFactory.java:91)
	at io.digdag.util.BaseOperator.run(BaseOperator.java:35)
	at io.digdag.core.agent.OperatorManager.callExecutor(OperatorManager.java:312)
	at io.digdag.cli.Run$OperatorManagerWithSkip.callExecutor(Run.java:694)
	at io.digdag.core.agent.OperatorManager.runWithWorkspace(OperatorManager.java:254)
	at io.digdag.core.agent.OperatorManager.lambda$runWithHeartbeat$2(OperatorManager.java:137)
	at io.digdag.core.agent.LocalWorkspaceManager.withExtractedArchive(LocalWorkspaceManager.java:25)
	at io.digdag.core.agent.OperatorManager.runWithHeartbeat(OperatorManager.java:135)
	at io.digdag.core.agent.OperatorManager.run(OperatorManager.java:119)
	at io.digdag.cli.Run$OperatorManagerWithSkip.run(Run.java:676)
	at io.digdag.core.agent.MultiThreadAgent.lambda$null$0(MultiThreadAgent.java:127)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:514)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:844)
2018-10-31 17:36:28 +0900 [INFO] (0019@[0:default]+a^failure-alert): type: notify
error:
  * +a+t2:
    Python command failed with code 1 (runtime)

Task state is saved at /Users/ariga/src/test-py3/.digdag/status/20181031T000000+0000 directory.
  * Use --session <daily | hourly | "yyyy-MM-dd[ HH:mm:ss]"> to not reuse the last session time.
  * Use --rerun, --start +NAME, or --goal +NAME argument to rerun skipped tasks.

After

➜  test-py3 ~/src/digdag/pkg/digdag-0.9.32-SNAPSHOT.jar run a.dig
2018-10-31 17:37:01 +0900: Digdag v0.9.31
2018-10-31 17:37:02 +0900 [WARN] (main): Using a new session time 2018-10-31T00:00:00+00:00.
2018-10-31 17:37:03 +0900 [INFO] (main): Using session /Users/ariga/src/test-py3/.digdag/status/20181031T000000+0000.
2018-10-31 17:37:03 +0900 [INFO] (main): Starting a new session project id=1 workflow name=a session_time=2018-10-31T00:00:00+00:00
2018-10-31 17:37:03 +0900 [INFO] (0017@[0:default]+a+t1): py>: a.ok
(111, 2, 3)
2018-10-31 17:37:04 +0900 [INFO] (0017@[0:default]+a+t2): py>: a.ng
(111, 222, 3)
Success. Task state is saved at /Users/ariga/src/test-py3/.digdag/status/20181031T000000+0000 directory.
  * Use --session <daily | hourly | "yyyy-MM-dd[ HH:mm:ss]"> to not reuse the last session time.
  * Use --rerun, --start +NAME, or --goal +NAME argument to rerun skipped tasks.

@muga Can you check this PR if you have a spare time?

@hiroyuki-sato
Copy link
Contributor

@chezou Thank you for fixing #912.
The original Reporter said LGTM 👍
Twitter

I'm not familiar Python.

@@ -122,7 +122,7 @@ def digdag_inspect_arguments(callable_type, exclude_self, params):
if key in params:
args[key] = params[key]
else:
if spec.defaults is None or len(spec.defaults) < idx:
if spec.defaults is None or idx < len(spec.args) - len(spec.defaults):
Copy link
Member Author

Choose a reason for hiding this comment

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

Just for note.
spec.args returns all arguments in a list, while spec.defaults returns a tuple of default values.

For example:

In [2]: def ng(arg1, arg2, arg3=3):
   ...:     print(arg1, arg2, arg3)

In [4]: spec = inspect.getargspec(ng)

In [5]: spec.args
Out[5]: ['arg1', 'arg2', 'arg3']

In [6]: spec.defaults
Out[6]: (3,)

In [7]: spec
Out[7]: ArgSpec(args=['arg1', 'arg2', 'arg3'], varargs=None, keywords=None, defaults=(3,))

Default arguments should be in the end of arguments, so we need to check front arguments.

@chezou
Copy link
Member Author

chezou commented Jan 10, 2019

@muga Just for reminder. Can you check it if you have a time?
It'd be nice if you could give some feedback; add test, something like that...

@szyn szyn requested a review from muga January 26, 2019 14:40
@muga
Copy link
Member

muga commented Jan 27, 2019

Sorry to be late. Let me take a look and will check CI failure.

@muga muga added the bug label Jan 27, 2019
@muga
Copy link
Member

muga commented Jan 27, 2019

Looks good. thank you for fixing. Let me merge after CI will be passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants