Refactor Http class to utilize ParameterHelper for http_params conver… #130

HarshitNagpal29 · 2024-01-02T09:46:34Z

…sion

fix: #88

Brief Summary of The Change

Pull Request checklist

I confirm that the following checklist has been completed.

Add/Change test cases for the changes.
Add/Change the related documentation, should also change docs/source/config.rst when you change file default_config.yaml.
(Optional) Add your change to UPDATING.md when it is an incompatible change.

…sion

zhongjiajie

please find my two cents. And BTW, could you please add some docstring for http tasks? we have some sample in
https://github.com/apache/dolphinscheduler-sdk-python/blob/48586a3702df3f708044c8c03eedd442f21cf24f/src/pydolphinscheduler/tasks/shell.py#L25C1-L44

src/pydolphinscheduler/tasks/http.py

zhongjiajie · 2024-01-03T02:37:39Z

BTW, since it is an incompatible change, we should add some comments in

dolphinscheduler-sdk-python/UPDATING.md

Line 29 in 1b2ad57

    
           * Drop support of python3.6 and python3.7 ([#126](https://github.com/apache/dolphinscheduler-sdk-python/pull/126))

we already have some example in that.

And we better add some DeprecationWarning for the user who still passes old parameter type for it. the old parameter seems like

http = Http(
        name="http",
        url="http://www.google.com",
        http_method="GET",
        http_params=[
            {"prop": "abc", "httpParametersType": "PARAMETER", "value": "def"}
        ],
    )

And we should raise some warnings and tell users that the format is deprecation

dolphinscheduler-sdk-python/src/pydolphinscheduler/core/task.py

Lines 202 to 207 in e1d1527

    
           if "process_definition" in kwargs: 
        
               warnings.warn( 
        
                   "The `process_definition` parameter is deprecated, please use `workflow` instead.", 
        
                   DeprecationWarning, 
        
               ) 
        
               self.workflow = kwargs.pop("process_definition")

maybe it can be

        if isinstance(http_params, list):
            warnings.warn(
                "The `http_params` parameter currently accept dict instead of list, you parameter is ignore.",
                DeprecationWarning,
            )

src/pydolphinscheduler/tasks/http.py

zhongjiajie · 2024-01-03T09:19:45Z

src/pydolphinscheduler/tasks/test_http_conversion.py

+from pydolphinscheduler.core.parameter import ParameterHelper, ParameterType
+from http.py import Http  # Replace 'your_module_path' with the actual path or import structure
+
+def test_http_params_conversion():


Great! you add some tests for this, but we already have tests for task http in https://github.com/apache/dolphinscheduler-sdk-python/blob/f82e07d232380fe9f5582db4dcca920ae00befd4/tests/tasks/test_http.py could you merge into that file?

zhongjiajie · 2024-01-03T09:20:43Z

src/pydolphinscheduler/tasks/http.py

+    Example:
+        Usage example for creating an HTTP task:
+        http_task = Http(name="http_task", url="https://api.example.com", http_method="POST", http_params={"key": "value"})


you add a good example 👍

zhongjiajie · 2024-01-03T09:22:21Z

src/pydolphinscheduler/tasks/http.py

+    Args:
+        name (str): The name or identifier for the HTTP task.
+        url (str): The URL endpoint for the HTTP request.
+        http_method (str, optional): The HTTP method for the request (GET, POST, etc.). Defaults to HttpMethod.GET.
+        http_params (str, optional): Parameters for the HTTP request. Defaults to None.
+        http_check_condition (str, optional): Condition for checking the HTTP response status. 
+                                               Defaults to HttpCheckCondition.STATUS_CODE_DEFAULT.
+        condition (str, optional): Additional condition to evaluate if `http_check_condition` is not STATUS_CODE_DEFAULT.
+        connect_timeout (int, optional): Connection timeout for the HTTP request in milliseconds. Defaults to 60000.
+        socket_timeout (int, optional): Socket timeout for the HTTP request in milliseconds. Defaults to 60000.


can we change the format to

:param name: A unique, meaningful string for the shell task. :param command: One or more command want to run in this task.

just like we did in shell task, in this format our docs template can auto fill the parameter type for our users

zhongjiajie · 2024-01-03T09:24:21Z

Hi @HarshitNagpal29, it is good to see you add some tests and fantastic docstring in this PR, well done 👍 +1
I had left some new comments for this PR, PTAL if you have time

HarshitNagpal29 · 2024-01-03T11:49:54Z

@zhongjiajie sir i have made all the changes as you mentioned.

HarshitNagpal29 · 2024-01-03T11:59:14Z

Hi @HarshitNagpal29, it is good to see you add some tests and fantastic docstring in this PR, well done 👍 +1 I had left some new comments for this PR, PTAL if you have time

sir please tell me what to do, i am happy to help

zhongjiajie · 2024-01-04T02:22:55Z

@zhongjiajie sir i am looking forward to participate in gsoc 2024 , i am new to open source contribution but i am well aware with javascript, node.js and somewhat python as well , i found your organisation pretty interesting and i am looking forward to contribute more, can you please provide some resources, places where i can connect with mentors more often , and some valuable feedback, i will be really grateful for that.
thank you

Great, do you have any resources about gsoc 2024? our project join gsoc 2022 but not in 2023, and I have act as mentor for 2022 too. I want to know the timeline about project submission.

can you please provide some resources, places where i can connect with mentors more often

you can find my email in my GitHub profile and connect me via that.

zhongjiajie

I add some addition comments, PTAL

UPDATING.md

tests/tasks/test_http.py

zhongjiajie · 2024-01-04T02:32:41Z

tests/tasks/test_http.py

+
+    # Add any assertions or additional tests as required based on your project's logic
+    assert isinstance(http_instance.http_params, list)
+    assert len(http_instance.http_params) == len(http_params_dict)


we should also add one more test for http_instance.http_params, cause this is the core concept about what we change in this PR. For example

expect = [ {"prop": "prop1", "direct": "IN", "type": "VARCHAR", "value": "value1"}, {"prop": "prop2", "direct": "IN", "type": "VARCHAR", "value": "value2"}, {"prop": "prop3", "direct": "IN", "type": "VARCHAR", "value": "value3"}, ] assert http.http_params == expect

tests/tasks/test_http.py

zhongjiajie · 2024-01-04T02:34:34Z

tests/tasks/test_http.py

@@ -126,3 +127,30 @@ def test_http_get_define():
    ):
        http = Http(name, url)
        assert http.task_params == expect_task_params
+
+
+def test_http_params_conversion():


due to you already copy and paste it from file test_http_conversion.py I thing you can remove the file test_http_conversion.py.

BTW we should add one more test for the warning when user still pass list to http task parameter http_params. we have an example in

dolphinscheduler-sdk-python/tests/tasks/test_sub_workflow.py

Lines 110 to 136 in f82e07d

def test_deprecated_sub_workflow_get_define(mock_workflow_definition):

"""Test deprecated task sub_process still work and raise warning."""

code = 123

version = 1

name = "test_sub_workflow_get_define"

expect_task_params = {

"resourceList": [],

"localParams": [],

"processDefinitionCode": TEST_SUB_WORKFLOW_CODE,

"dependence": {},

"conditionResult": {"successNode": [""], "failedNode": [""]},

"waitStartTimeout": {},

}

with patch(

"pydolphinscheduler.core.task.Task.gen_code_and_version",

return_value=(code, version),

):

with warnings.catch_warnings(record=True) as w:

from pydolphinscheduler.tasks.sub_process import SubProcess

assert len(w) == 1

assert issubclass(w[-1].category, DeprecationWarning)

assert "deprecated" in str(w[-1].message)

with Workflow(TEST_WORKFLOW_NAME):

sub_workflow = SubProcess(name, TEST_SUB_WORKFLOW_NAME)

assert sub_workflow.task_params == expect_task_params

wish it can help you

src/pydolphinscheduler/tasks/http.py

zhongjiajie · 2024-01-04T02:44:02Z

BTW, we have a code style check for our codebase, you could run the command tox -e auto-lint to format the code. for more information, you can see in https://github.com/apache/dolphinscheduler-sdk-python/blob/main/CONTRIBUTING.md#code-style-using-tox

and for how to set tox development you can see in https://github.com/apache/dolphinscheduler-sdk-python/blob/main/CONTRIBUTING.md#automated-testing-with-tox

zhongjiajie · 2024-01-04T06:42:47Z

And I have commit and change some code to you branch, so you should pull them to your local before you add your change

HarshitNagpal29 · 2024-01-04T13:36:40Z

@zhongjiajie sir i have made the changes and added some tests as well suggested by you.

HarshitNagpal29 · 2024-01-04T14:08:08Z

@zhongjiajie sir i have mailed you to connect with you

HarshitNagpal29 · 2024-01-06T07:29:45Z

@zhongjiajie sir I am waiting for your response

zhongjiajie

Other LGTM, but it seems you forget to format the code by too -e auto-lint

zhongjiajie · 2024-01-06T11:17:51Z

tests/tasks/test_http.py

+    expect = [
+        {"prop": "prop1", "direct": "IN", "type": "VARCHAR", "value": "value1"},


Hi, we can merge test function test_http_params_transformation and test_http_params_conversion into single one

Hi, we can merge test function test_http_params_transformation and test_http_params_conversion into single one

ok i am merging these 2 test functions and pushing it again in few minutes.

zhongjiajie · 2024-01-06T11:18:41Z

I approval the CI run

HarshitNagpal29 · 2024-01-06T11:38:45Z

Other LGTM, but it seems you forget to format the code by too -e auto-lint

Sorry for the inconvenience i will try to do it now.

zhongjiajie · 2024-01-06T11:58:16Z

you can run the command and then append one more commit this this PR

HarshitNagpal29 · 2024-01-07T01:05:24Z

@zhongjiajie sir i don't know why but i am not able to setup and run tox command on my local machine, I have tried a lot really sorry for that but still I have done the formatting, linting, styling and all using black and ruff commands manually, please check if it still needs improvement, again sorry for inconvenience.

zhongjiajie · 2024-01-07T01:55:56Z

I approval CI run.
what OS do you use in your local machine?

HarshitNagpal29 · 2024-01-07T05:48:22Z

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

zhongjiajie · 2024-01-07T11:45:20Z

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

tax should work with macOS, and you should format http.py files because log said

lint: 15588 W commands[0]> python -m black --check . [tox/tox_env/api.py:427]
[549](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:550)
would reformat /home/runner/work/dolphinscheduler-sdk-python/dolphinscheduler-sdk-python/src/pydolphinscheduler/tasks/http.py
[550](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:551)

[551](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:552)
Oh no! 💥 💔 💥
[552](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:553)
1 file would be reformatted, 157 files would be left unchanged.

zhongjiajie · 2024-01-07T11:46:06Z

do you install tox first then trying run command tox -e auto-lint? and how about the output when you run tox -l?

HarshitNagpal29 · 2024-01-07T14:29:27Z

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

tax should work with macOS, and you should format http.py files because log said

lint: 15588 W commands[0]> python -m black --check . [tox/tox_env/api.py:427]
[549](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:550)
would reformat /home/runner/work/dolphinscheduler-sdk-python/dolphinscheduler-sdk-python/src/pydolphinscheduler/tasks/http.py
[550](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:551)

[551](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:552)
Oh no! 💥 💔 💥
[552](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:553)
1 file would be reformatted, 157 files would be left unchanged.

@zhongjiajie sir I made changes in http.py file using tox , I used different machine with linux as OS , in my mac machine even after installing tox when I was inputting "tox -l" command, it was showing me only version of python that is "312" no file like tox.ini and all, maybe it was some kind of a glitch.

zhongjiajie · 2024-01-08T06:22:24Z

maybe you can remove the project and try clone from the GitHub again after this PR merged

HarshitNagpal29 · 2024-01-08T06:29:30Z

@zhongjiajie ok I will try that method, but do you need any more changes in this pull request

zhongjiajie · 2024-01-08T06:51:18Z

lint still no pass, maybe you can run command python -m ruff check --fix --unsafe-fixes . to fix it

lint: 16342 W commands[1]> python -m ruff check . [tox/tox_env/api.py:427]
[553](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:554)
tests/tasks/test_http.py:167:13: F841 Local variable `http` is assigned to but never used
[554](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:555)
Found 1 error.
[555](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:556)
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

HarshitNagpal29 · 2024-01-08T09:42:14Z

@zhongjiajie sir please check it now, i fixed that linting issue and in my machine it is showing me everything fine

HarshitNagpal29 · 2024-01-08T12:32:47Z

@zhongjiajie sir please provide approval now

zhongjiajie · 2024-01-09T01:47:02Z

test error for case test_http_params_deprecation_warning with link https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7445619872/job/20284481402?pr=130

codecov-commenter · 2024-01-09T01:59:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f16599f) 90.86% compared to head (3a22ec0) 90.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   90.86%   90.89%   +0.02%     
==========================================
  Files          64       64              
  Lines        2354     2361       +7     
==========================================
+ Hits         2139     2146       +7     
  Misses        215      215

Flag	Coverage Δ
unittests	`90.89% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

zhongjiajie · 2024-01-09T02:02:11Z

Hi @HarshitNagpal29 I add and fix some tests in 3a22ec0

for the test_http_params we need to mock the return of function gen_code_and_version which is called with dolphinscheduler code in apache/dolphinscheduler

@patch(
    "pydolphinscheduler.core.task.Task.gen_code_and_version",
    return_value=(123, 1),
)

and for case test_http_params_deprecation_warning, w[-1].message is detail of warnings message, which depend on your warning message in the class Http

zhongjiajie · 2024-01-09T02:03:39Z

Now all CI are green, merging 🎉 thanks for your contribution and looking forward your next contribute

HarshitNagpal29 · 2024-01-09T03:39:11Z

@zhongjiajie sir i am really grateful that i was able to work on this issue with you and learn lot of new things, i am looking forward to contribute more , can you please suggest some good first issues which i can work upon or maybe some documentation changes in this project or some other.

zhongjiajie · 2024-01-09T03:53:04Z

Hi @HarshitNagpal29 here are two of issue maybe good and easy to fix

Docs changed only, you can learn how our docs build and render [Feature][python] Add example about multi line shell dolphinscheduler#8955
Code change, which you need to add utils class to detect and setValue and auto add out-put parameter for our code [impv] Auto check and add output local_params if we define the special grammar in the task definition #61

Refactor Http class to utilize ParameterHelper for http_params conver…

b92af11

…sion

zhongjiajie reviewed Jan 3, 2024

View reviewed changes

src/pydolphinscheduler/tasks/http.py Outdated Show resolved Hide resolved

zhongjiajie assigned HarshitNagpal29 Jan 3, 2024

zhongjiajie added the incompatible label Jan 3, 2024

made all the changes

7e84357

zhongjiajie reviewed Jan 3, 2024

View reviewed changes

HarshitNagpal29 added 2 commits January 3, 2024 17:10

added tests

fe43a7f

made changes

20eb2d0

zhongjiajie reviewed Jan 4, 2024

View reviewed changes

zhongjiajie added 3 commits January 4, 2024 10:40

Update UPDATING.md

6647a36

Update tests/tasks/test_http.py

1716046

Update src/pydolphinscheduler/tasks/http.py

a1e69aa

HarshitNagpal29 added 2 commits January 4, 2024 18:51

done

f7b9152

file deleted

170bde6

zhongjiajie reviewed Jan 6, 2024

View reviewed changes

code styling done

cd1c1ab

code styling of http.py using tox

2dee783

fixed linting issues in test_http.py

71a2836

fix test

3a22ec0

zhongjiajie added the enhancement New feature or request label Jan 9, 2024

zhongjiajie merged commit 0f120f6 into apache:main Jan 9, 2024
22 checks passed

HarshitNagpal29 deleted the feature/http-parameter-refactor branch January 9, 2024 11:15

	def test_deprecated_sub_workflow_get_define(mock_workflow_definition):
	"""Test deprecated task sub_process still work and raise warning."""
	code = 123
	version = 1
	name = "test_sub_workflow_get_define"
	expect_task_params = {
	"resourceList": [],
	"localParams": [],
	"processDefinitionCode": TEST_SUB_WORKFLOW_CODE,
	"dependence": {},
	"conditionResult": {"successNode": [""], "failedNode": [""]},
	"waitStartTimeout": {},
	}
	with patch(
	"pydolphinscheduler.core.task.Task.gen_code_and_version",
	return_value=(code, version),
	):
	with warnings.catch_warnings(record=True) as w:
	from pydolphinscheduler.tasks.sub_process import SubProcess

	assert len(w) == 1
	assert issubclass(w[-1].category, DeprecationWarning)
	assert "deprecated" in str(w[-1].message)

	with Workflow(TEST_WORKFLOW_NAME):
	sub_workflow = SubProcess(name, TEST_SUB_WORKFLOW_NAME)
	assert sub_workflow.task_params == expect_task_params

		expect = [
		{"prop": "prop1", "direct": "IN", "type": "VARCHAR", "value": "value1"},

Refactor Http class to utilize ParameterHelper for http_params conver… #130

Refactor Http class to utilize ParameterHelper for http_params conver… #130

Conversation

HarshitNagpal29 commented Jan 2, 2024 • edited by zhongjiajie

Brief Summary of The Change

Pull Request checklist

zhongjiajie left a comment

Choose a reason for hiding this comment

zhongjiajie commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongjiajie commented Jan 3, 2024

HarshitNagpal29 commented Jan 3, 2024

HarshitNagpal29 commented Jan 3, 2024

zhongjiajie commented Jan 4, 2024 • edited

zhongjiajie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongjiajie commented Jan 4, 2024

zhongjiajie commented Jan 4, 2024

HarshitNagpal29 commented Jan 4, 2024

HarshitNagpal29 commented Jan 4, 2024

HarshitNagpal29 commented Jan 6, 2024

zhongjiajie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongjiajie commented Jan 6, 2024

HarshitNagpal29 commented Jan 6, 2024

zhongjiajie commented Jan 6, 2024

HarshitNagpal29 commented Jan 7, 2024

zhongjiajie commented Jan 7, 2024

HarshitNagpal29 commented Jan 7, 2024

zhongjiajie commented Jan 7, 2024

zhongjiajie commented Jan 7, 2024 • edited

HarshitNagpal29 commented Jan 7, 2024

zhongjiajie commented Jan 8, 2024

HarshitNagpal29 commented Jan 8, 2024

zhongjiajie commented Jan 8, 2024

HarshitNagpal29 commented Jan 8, 2024

HarshitNagpal29 commented Jan 8, 2024

zhongjiajie commented Jan 9, 2024

codecov-commenter commented Jan 9, 2024 • edited

Codecov Report

zhongjiajie commented Jan 9, 2024

zhongjiajie commented Jan 9, 2024

HarshitNagpal29 commented Jan 9, 2024

zhongjiajie commented Jan 9, 2024

HarshitNagpal29 commented Jan 2, 2024 •

edited by zhongjiajie

zhongjiajie commented Jan 4, 2024 •

edited

zhongjiajie commented Jan 7, 2024 •

edited

codecov-commenter commented Jan 9, 2024 •

edited