Dataops 832 olink cluster info #16
Conversation
nkongenelly
force-pushed
the
DATAOPS-832_olink_cluster_info
branch
2 times, most recently
from
3245ce8 to
9404ae1
Compare
matrulda
left a comment
matrulda
left a comment
There was a problem hiding this comment.
Great! In addition to my other comments, could you remove all Interop/CX.X folders? I'm pretty sure only the binaries on the root level of InterOp are needed.
.travis.yml
Outdated
| language: python | ||
| python: | ||
| - 3.6 | ||
| - 3.10 |
There was a problem hiding this comment.
Great, but could you remove this file and instead add a GHA workflow for running the tests?
nkongenelly
force-pushed
the
DATAOPS-832_olink_cluster_info
branch
2 times, most recently
from
64397f2 to
a888b31
Compare
nkongenelly
force-pushed
the
DATAOPS-832_olink_cluster_info
branch
from
a2ee4e4 to
cb1ac93
Compare
|
@matrulda , thanks for the review. I have now pushed the updated code |
matrulda
left a comment
matrulda
left a comment
There was a problem hiding this comment.
Great, thanks for addressing my changes! I left some comments for you.
.github/workflows/unit_tests.yaml
Outdated
| - name: Launch tests | ||
| run: | | ||
| pytest --cov=checkQC --cov-report=xml tests/ |
There was a problem hiding this comment.
This is not CheckQC :) Also, projman_filler has not been configured to use codecov. I think we should skip the codecov pats for now, we can add it later.
README.md
Outdated
| Development and testing | ||
| ----------------------- | ||
| Set up a Python environment using the latest version of Python 3.6 (3.6.12 as of this writing). | ||
| Set up a Python environment using the latest version of Python 3.10 (3.10.1 as of this writing). |
There was a problem hiding this comment.
3.10.15 is the latest version of python 3.10? Either way, I don't think we have to mention the latest version as of writing, so I think you can remove the whole parenthesis
projman_filler/__init__.py
Outdated
| """Top-level package for ProjMan Filler.""" | ||
|
|
||
| __version__ = '1.6.3' | ||
| __version__ = '1.6.4' |
There was a problem hiding this comment.
I wonder if it should be at least a minor version update since the python requirement have changed.
There was a problem hiding this comment.
I think the cache directory can be removed.
| interop.summary = MagicMock() | ||
|
|
||
| def test_interop_standardize_read_numbers(self): | ||
| self._setup_mocks() |
There was a problem hiding this comment.
Please remove the _setup_mocks as well.
|
Oh one more thing, could you look into updating the wheel dependency while at it? https://github.com/Molmed/projman_filler/security/dependabot/4 |
nkongenelly
force-pushed
the
DATAOPS-832_olink_cluster_info
branch
2 times, most recently
from
4d5559c to
bab969a
Compare
|
Thank you for the review. I have now pushed the updated code |
matrulda
left a comment
matrulda
left a comment
There was a problem hiding this comment.
Looks good to me.
Note that we do not have a staging environment for Projman filler, so it is best to validate it locally using Seq-summaries and connect to Projman_devel.
setup.py
Outdated
| 'Click~=8.1.3', | ||
| 'xmltodict~=0.13.0', | ||
| 'interop~=1.2.3', | ||
| 'interop~=1.3.2', | ||
| 'SQLAlchemy~=1.4.48', | ||
| 'pymssql~=2.2.7', | ||
| 'pandas~=1.1.5' | ||
| 'pandas~=2.2.3', | ||
| 'numpy~=1.24.2' |
There was a problem hiding this comment.
Maybe we could update all dependencies to their latest version?
| 'Intended Audience :: Developers', | ||
| 'License :: OSI Approved :: GNU General Public License v3 (GPLv3)', | ||
| 'Natural Language :: English', | ||
| 'Programming Language :: Python :: 3.6', |
There was a problem hiding this comment.
Is there a reason for stopping at 3.10 and not going to a higher version?
There was a problem hiding this comment.
Not too sure. The reason we went with python 3.10 the default python version on the ubuntu server running this is py3.10. I hope i got this right from the conversation with @matrulda
There was a problem hiding this comment.
Might you have any other suggestion on this @Aratz ?
There was a problem hiding this comment.
Ah yes, because before projman_filler didn't have a virtual env and just ran directly on the system (unclear why). Now we use pyenv so we could try using a newer version. Interop has been updated to support 3.13 https://github.com/Illumina/interop/releases/tag/v1.4.0, so we could try that.
|
Thanks for the reviews. I have now bumped up python version to 3.13.1. and updated dependencies. |
3 participants
It was discovered that the dataframe had the wrong index per lane and needed to be reset:
See ticket comment for screenshots of how the index was before and after