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

release: 0.8.1 #219

Merged
merged 5 commits into from
Feb 7, 2022
Merged

release: 0.8.1 #219

merged 5 commits into from
Feb 7, 2022

Conversation

mvidalgarcia
Copy link
Member

@mvidalgarcia mvidalgarcia commented Feb 1, 2022

closes #220
closes #221

@codecov-commenter
Copy link

Codecov Report

Merging #219 (437aad2) into master (ac194f6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #219   +/-   ##
=======================================
  Coverage   44.92%   44.92%           
=======================================
  Files           6        6           
  Lines         256      256           
=======================================
  Hits          115      115           
  Misses        141      141           
Impacted Files Coverage Δ
reana_workflow_engine_yadage/version.py 100.00% <100.00%> (ø)

requirements.txt Outdated
vine==1.3.0 # via amqp
webcolors==1.11.1 # via jsonschema
werkzeug==2.0.2 # via reana-commons
yadage-schemas==0.10.6 # via packtivity, reana-commons, yadage
yadage-schemas==0.10.7 # via packtivity, reana-commons, yadage
yadage==0.20.1 # via reana-commons
Copy link

@matthewfeickert matthewfeickert Feb 4, 2022

Choose a reason for hiding this comment

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

Sorry to ask for yet another rebuild of the requirements.txt, but can you rebuild it with yadage==0.20.2 given reanahub/reana-commons#333?

This should get picked up automatically if your just have pip-tools recompile as the last time this requirements.txt got built yadage v0.20.2 wasn't out yet.

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewfeickert After upgrading, Yadage workflows are failing again:

$ reana-client logs -w helloworld-yadage-kubernetes     
==> Workflow engine logs
Workflow exited unexpectedly.
stat: path should be string, bytes, os.PathLike or integer, not NoneType

Do you have any clue?

Copy link

@matthewfeickert matthewfeickert Feb 4, 2022

Choose a reason for hiding this comment

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

Can you tell me:

  • What installed packaged changed beyond yadage, if any?
  • If there are any more detailed logs?
  • If there is a minimal reproducible example that I can run myself for faster debugging purposes?

Copy link
Member Author

@mvidalgarcia mvidalgarcia Feb 4, 2022

Choose a reason for hiding this comment

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

It works as expected with these changes:

diff --git a/requirements.txt b/requirements.txt
index c20ed6b..31b6cc3 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -41,7 +41,7 @@ pyrsistent==0.18.0        # via jsonschema
 python-dateutil==2.8.2    # via bravado, bravado-core
 pytz==2021.3              # via bravado-core, fs
 pyyaml==5.4.1             # via bravado, bravado-core, packtivity, reana-commons, yadage, yadage-schemas
-reana-commons[yadage]==0.8.3  # via reana-workflow-engine-yadage (setup.py)
+reana-commons[yadage]==0.8.3a1  # via reana-workflow-engine-yadage (setup.py)
 requests[security]==2.26.0  # via bravado, packtivity, reana-workflow-engine-yadage (setup.py), yadage, yadage-schemas
 rfc3987==1.3.8            # via jsonschema, reana-workflow-engine-yadage (setup.py)
 simplejson==3.17.5        # via bravado, bravado-core
@@ -54,7 +54,7 @@ vine==1.3.0               # via amqp
 webcolors==1.11.1         # via jsonschema
 werkzeug==2.0.2           # via reana-commons
 yadage-schemas==0.10.6    # via packtivity, reana-commons, yadage
-yadage==0.20.2            # via reana-commons
+yadage==0.20.1            # via reana-commons
 
 # The following packages are considered to be unsafe in a requirements file:
 # setuptools
diff --git a/setup.py b/setup.py
index 7933c33..505c852 100644
--- a/setup.py
+++ b/setup.py
@@ -47,7 +47,7 @@ install_requires = [
     "mock>=3.0,<4",
     "pydot>=1.0.29",  # FIXME needed only if yadage visuale=True.
     "pydotplus>=2.0.2",  # FIXME needed only if yadage visuale=True.
-    "reana-commons[yadage]>=0.8.3,<0.9.0",
+    "reana-commons[yadage]==0.8.3a1",
     "requests>=2.25.1",
     "rfc3987==1.3.8",  # FIXME remove once yadage-schemas solves yadage deps.
 ]

Is there anything we can do to fix it?

Copy link

@matthewfeickert matthewfeickert Feb 4, 2022

Choose a reason for hiding this comment

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

It works as expected with these changes:

Is the

-reana-commons[yadage]==0.8.3  # via reana-workflow-engine-yadage (setup.py)
+reana-commons[yadage]==0.8.3a1  # via reana-workflow-engine-yadage (setup.py)

relevant at all? Or is that just an artifact of the diff? Things work with reana-commons[yadage]==0.8.3 and yadage==0.20.1, and they don't work with reana-commons[yadage]==0.8.3 and yadage==0.20.2, but do they work with reana-commons[yadage]==0.8.3a1 and yadage==0.20.2? (Sorry had to edit this comment a few times to get the release numbers right.)

Is there anything we can do to fix it?

I can yank the release if we aren't able to understand what is happening and then have you roll back the relevant part of reanahub/reana-commons#333, but I think the fastest way forward is if there is a way for me to be able to get an example or an environment where I can test this and see what's actually happening in the logs.

Copy link
Member

Choose a reason for hiding this comment

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

hi @mvidalgarcia is there some fairly isolated engine-specific test suite / setup that would allow @matthewfeickert do test the setup with the new releases short of deploying a full REANA cluster?

Copy link
Member

Choose a reason for hiding this comment

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

I think we have basically three options:

A. Enrich Yadage's own test suite to have all the functions that REANA uses covered and tested there. This may be tricky, since r-w-e-yadage uses a lot of functions from yadage, adage, yadageschemas, and packtivity, and some of them may be perhaps considered Yadage's "internal functions" and not Yadage's "API-like" functions in purpose. Examples include: steering_ctx, dagstate, finalize_inputs, and more. We'd have to go carefully through r-w-e-yadage integration code and make a list and make sure that all these usages are well covered in Yadage's own internal test suite. This would take time, and may be fragile if either Yadage implementation code changes or REANA changes code without updating one another.

B. Deploy local REANA cluster with new Yadage versions via Kind and test there. This is the fully opposite option, not to test in Yadage but to test in REANA. This is more natural, and we do have reana-dev helper script which helps in setting things up. We could further build a tiny shell script wrapper to make it even easier to automatise. However it would require running Kind etc, so it would bring some REANA development environment to learn, which you were seeking to avoid.

C. Release constraint on REANA client and REANA server pinning the same Yadage versions. This would allow incrementing Yadage versions more freely between client and cluster. Which in my eyes would be probably the best solution, since it would both (i) simplify option B and (ii) allow for using any local Yadage version that the user may have installed, whilst still connecting to any compatible remote REANA's Yadage version supported by the cluster. I think this should mostly work, there is no reason why two different Yadage 0.20.x wouldn't be able to run the same workflow definition!
The option C, besides relaxing unnecessary Yadage version constraint to the users on the client side, would allow us to implement client in a faster language (e.g. Go) whilst moving all the advanced workflow validation functionality to the server-side (in order to run on the exactly the same Yadage version that the remote cluster supports).

TL;DR The option A moves all testing to Yadage side, the option B moves all testing to the REANA side, the option C tries to dissociate Yadage clients from REANA Yadage server, so that both can keep evolving independently of each other.

Copy link

@matthewfeickert matthewfeickert Feb 8, 2022

Choose a reason for hiding this comment

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

@tiborsimko Thanks for the summary.

The yadage org libraries have bad codecoverage at the moment. I don't think that's going to drastically improve quickly as @lukasheinrich and I are both more "keeping things working" maintainers at the moment rather than active developers. So while GitHub Issues that contain the information of

go carefully through r-w-e-yadage integration code and make a list and make sure that all these usages are well covered in Yadage's own internal test suite.

would be welcome and useful that's a lot of time and work on the REANA team side of things.

Deploy local REANA cluster with new Yadage versions via Kind and test there

I like this idea in general as it seems to be the natural relationship between an application (REANA) and libraries (everything under the yadage org) of testing any library before release and deployment. This would also give reproducible examples of any bugs and logs which are needed for any debugging.

I don't have any thoughts on your suggested Option C as that seems to be a decision for the REANA team in general, but yes, I agree with the sentiment that

there is no reason why two different Yadage 0.20.x wouldn't be able to run the same workflow definition!

(btw we've yanked yadage v0.20.2 from PyPI so that it can't cause additional problems anywhere. We'll release v0.20.3 once we have a fix that we've tested.)

`pygraphviz==1.8` dropped support for py37 and RTD builds the documentation using py37.
A `pygraphviz` is not relevant to build the docs, it can be separated in an extra.
- docker image uses py38
- in the setup's classifiers when only mention version py38

closes reanahub#221
@mvidalgarcia mvidalgarcia force-pushed the release branch 2 times, most recently from 943f208 to 472b4ae Compare February 7, 2022 10:53
- `yadage==0.20.2` is failing at the workflow engine level
- it requires to pin reana-commons to 0.8.3.a1, since 0.8.3 requires `yadage~=0.20.2`
Copy link
Member

@audrium audrium left a comment

Choose a reason for hiding this comment

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

LGTM! Temporary pinning reana-commons[yadage]==0.8.3a1 solves the issue

@tiborsimko
Copy link
Member

FWIW pinning alpha package r-commons 0.8.3a1 in non-alpha r-w-e-yadage 0.8.1 does not seem very good, because it leads to conflicts:

$ docker build -t reanahub/reana-workflow-engine-yadage:latest .
---> Running in 29ffcf96a0ac
reana-workflow-engine-yadage 0.8.1 has requirement reana-commons[yadage]==0.8.3a1, but you have reana-commons 0.8.3.
The command '/bin/sh -c pip check' returned a non-zero code: 1

when one has r-commons 0.8.3 checked out locally for r-client 0.8.1 PR...

audrium added a commit to audrium/reana-commons that referenced this pull request Feb 8, 2022
audrium added a commit to audrium/reana-commons that referenced this pull request Feb 8, 2022
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.

Add python_requires=">=3.8" setup: upgrading to adage==0.10.2 breaks yadage workflows
6 participants