Skip to content

Add dagexecute support #69

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 56 commits into from
Jul 21, 2021
Merged

Add dagexecute support #69

merged 56 commits into from
Jul 21, 2021

Conversation

AvitalFineRedis
Copy link
Contributor

No description provided.

alonre24 and others added 17 commits April 26, 2021 17:01
@AvitalFineRedis AvitalFineRedis linked an issue May 21, 2021 that may be closed by this pull request
add support for DAGEXECUTE #67
Closed
@codecov
Copy link

codecov bot commented May 23, 2021 •
edited
Loading

Codecov Report

Merging #69 (9a4415c) into master (53b1106) will decrease coverage by 2.00%.
The diff coverage is 82.50%.

❗ Current head 9a4415c differs from pull request most recent head c934940. Consider uploading reports for the commit c934940 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   97.40%   95.39%   -2.01%     
==========================================
  Files           8        8              
  Lines         848      826      -22     
==========================================
- Hits          826      788      -38     
- Misses         22       38      +16
Impacted Files Coverage Δ
redisai/client.py 96.96% <ø> (-0.15%) ⬇️
redisai/dag.py 88.00% <76.47%> (-10.08%) ⬇️
test/test.py 98.32% <86.95%> (-1.45%) ⬇️
redisai/pipeline.py 84.84% <0.00%> (-15.16%) ⬇️
redisai/utils.py 97.50% <0.00%> (ø)
redisai/postprocessor.py 100.00% <0.00%> (ø)
redisai/command_builder.py 89.68% <0.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8182aa6...c934940. Read the comment docs.

@AvitalFineRedis AvitalFineRedis marked this pull request as ready for review May 23, 2021 07:08
alonre24
alonre24 reviewed Jul 18, 2021
View reviewed changes
Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Looks good! There is an empty file called test-requirments.txt - remove it. Tests are good, I would add one more complex test of using Redis commands in a script within a DAG.

redisai/dag.py Outdated
self.result_processors = []
self.enable_postprocess = postprocess
self.enable_postprocess = True
if load is None and persist is None and routing is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we are not backward compatible if we put this validation here (as AI.DAGRUN is allowed without any of these args). So this check should be moved to the execute method.

@AvitalFineRedis AvitalFineRedis requested a review from alonre24 July 19, 2021 12:43
alonre24
alonre24 reviewed Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Great job on the backward compatibility support! I Added some minor suggestions for clarifying the error messages. Last thing, please add a test for DAG with SCRIPEXECUTE that uses Redis commands.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@AvitalFineRedis AvitalFineRedis merged commit a10452a into master Jul 21, 2021
@AvitalFineRedis AvitalFineRedis deleted the Add_DAGEXECUTE_support branch July 21, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alonre24 alonre24 alonre24 approved these changes

Assignees

@alonre24 alonre24

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add support for DAGEXECUTE add script run to DAG SCRIPTRUN support on DAGRUN and DAGRUN_RO
2 participants
@AvitalFineRedis @alonre24