Add workflow to deploy DALiuGE to PyPI on Release #311
Conversation
Reviewer's Guide by SourceryThis pull request introduces a CI workflow to automatically build and publish the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Restrict PyPI publishing to releases.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @myxie - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a single workflow file to build and publish all three packages to reduce duplication.
- Instead of skipping the
test_bash_shell_portstest, try to fix it.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if inspect.stack()[1][3] == f.__name__: | ||
| if (spec and spec(args)) or not spec: | ||
| raise TailRecursion(args, kwargs) |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if inspect.stack()[1][3] == f.__name__: | |
| if (spec and spec(args)) or not spec: | |
| raise TailRecursion(args, kwargs) | |
| if inspect.stack()[1][3] == f.__name__ and ((spec and spec(args)) or not spec): | |
| raise TailRecursion(args, kwargs) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
|
|
||
| def test_switch(): | ||
| with pyext.switch('x'): | ||
| if case('x'): x = 4 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| def test_switch(): | ||
| with pyext.switch('x'): | ||
| if case('x'): x = 4 | ||
| if case('b'): x = 2 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| with pyext.switch('x'): | ||
| if case('x'): x = 4 | ||
| if case('b'): x = 2 | ||
| if case(1): x = 3 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if case('x'): x = 4 | ||
| if case('b'): x = 2 | ||
| if case(1): x = 3 | ||
| if case('a'): x = 1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if len(kw) == 0: | ||
| cargs = args | ||
| elif len(kw) == 1 and 'is_cls' in kw and kw['is_cls']: | ||
| cargs = args[1:] | ||
| else: | ||
| raise ValueError('Invalid keyword args specified') | ||
| types = _gettypes(cargs) | ||
| if types not in overloads: | ||
| raise TypeError(\ | ||
| "No overload of function '%s' that takes: %s" % ( | ||
| f.__name__, types)) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Simplify sequence length comparison (
simplify-len-comparison) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring)
| if len(args) >= 1: | ||
| f.__annotations__['return'] = args[0] |
There was a problem hiding this comment.
issue (code-quality): Simplify sequence length comparison (simplify-len-comparison)
| vsplit = list(map(str.strip, varname.split('.'))) | ||
| fvars = dict(fd, **fl) | ||
| if vsplit[0] not in fvars: | ||
| raise NameError('Unknown object: %s' % vsplit[0]) | ||
| base = fvars[vsplit[0]] | ||
| for x in vsplit[1:-1]: | ||
| base = getattr(base, x) | ||
| setattr(base, vsplit[-1], value) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into function (
extract-method) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring)
| def f(a): return str | ||
| @pyext.overload.args() | ||
| def f(): return | ||
| assert f() == None |
There was a problem hiding this comment.
issue (code-quality): Use x is None rather than x == None (none-compare)
| if case('x'): x = 4 | ||
| if case('x'): x = 2; case.quit() |
There was a problem hiding this comment.
suggestion (code-quality): Merge repeated if statements into single if (merge-repeated-ifs)
| if case('x'): x = 4 | |
| if case('x'): x = 2; case.quit() | |
| if case('x'): | |
| x = 4 | |
| x = 2 | |
| case.quit() |
| @@ -68,7 +68,7 @@ def _create_and_run_graph_spec_from_lgt(self, logical_graph_name: str): | |||
| roots = graph_loader.createGraphFromDropSpecList(appDropSpec) | |||
| # drops = [v for d,v in drops.items()] | |||
| leafs = droputils.getLeafNodes(roots) | |||
| with droputils.DROPWaiterCtx(self, leafs, timeout=3): | |||
| with droputils.DROPWaiterCtx(self, leafs, timeout=600): | |||
There was a problem hiding this comment.
Does the timeout really need to be that long? If, for some reason this gets into a hanging state, the test suite will sit there for 10 min before it continues. I had that a few times through vscode and it is really not helpful.
There was a problem hiding this comment.
It absolutely does not - I did this for debugging purposes but accidentally committed the edit. I will revert these changes.
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.9" |
There was a problem hiding this comment.
fairly old version of Python
There was a problem hiding this comment.
Good point, I will update to 3.10.
JIRA Ticket
Type
Problem/Issue
We would like to setup Continuous Deployment (CD) to PyPI for DALiuGE releases, to avoid having to do so manually.
We have not released for quite some time (~10 months), which means that some updates we've made to improve some things have broken our ability to build/push to PyPI. We have also been reliant on some direct installation links, which are not supported by PyPI.
This PR aims to address these issues at the same time as providing a minimal viable workflow that builds and pushes the daliuge modules to PyPI.
Solution
I have decided to make the PyPI run when we create a Release on GitHub. This is different to running the pipeline whenever we push a new tagged version, and (currently) requires a small amount of manual intervention (i.e. creating a Release through the GitHub UI). We do not have any "releases" on GitHub.
I have done this for a couple of reasons:
The work included in this PR includes:
Added the new publish_to_pypi.yml workflow file.
on: push.In response to LIU-449 Improved installation info #307 (comment) I have added string-literal VERSION files for daliuge-engine and daliuge-translator. This is to ensure that we are not relying on any dynamic version numbering at runtime.
Updated our
merklelibinstall to the newly releasednp-merklelibfork on PyPI.Added the source code from
PyExtto our code base and added the relevant tests/licenses, including a fix currently in a PR on the inactive repository.Removed the redundant test-requirements.txt file, which serves only to obfuscate the test requirements for the daliuge-translator module. Test dependencies are now tracked in the
extra_requiresdictionary.Checklist
Summary by Sourcery
This pull request introduces a CI workflow to automatically build and publish the
daliuge-common,daliuge-engine, anddaliuge-translatorpackages to TestPyPI upon a push event. It also updates the build process to use thebuildpackage and standardizes versioning across all packages.New Features:
daliuge-common,daliuge-engine, anddaliuge-translatorpackages to TestPyPI upon a push event.Enhancements:
buildpackage instead ofsetuptoolsdirectly.VERSIONfile, ensuring consistency across all packages.daliuge-commonduring the installation ofdaliuge-engineanddaliuge-translator.merklelibwithnp-merklelibindaliuge-common.pyextindaliuge-engineand includes the code directly in the repository.Build:
CI:
Chores: