search

yigbt / deepFPlearn

Link molecular structures of chemicals (in form of topological fingerprints) with multiple targets
Other
8 stars 6 forks source link

Add a github actions workflow for PR #12

Closed bernt-matthias closed 1 year ago

bernt-matthias commented 2 years ago

So for each PR the workflow will check python linting and pytests is successful.

bernt-matthias commented 2 years ago

test fals currently because requirements.txt will be added only in https://github.com/yigbt/deepFPlearn/pull/11

halirutan commented 1 year ago

Looks good to me. There is one thing: Checking the order of imports with isort lets the action fail. I have no strong preference, but usually I import system libs first and references into our own package after that. If you also tend towards this, then you need to get rid of the isort checking. Otherwise, the imports where the "check sort action" breaks need to be fixed.

I think it's ready to be merged after that. Sorry, for taking so long. "Someone" had to remind me.

bernt-matthias commented 1 year ago

There is one thing: Checking the order of imports with isort lets the action fail. I have no strong preference, but usually I import system libs first and references into our own package after that. If you also tend towards this, then you need to get rid of the isort checking.

This is fine .. isort does exactly this.

bernt-matthias commented 1 year ago

Now all works, except for typing :( Seems not trivial to fix. Should we:

1) skip for now and open a separate PR what adds and fixes typing 2) fix it here?

halirutan commented 1 year ago

Maybe this is a stupid question, but why is mypy not in the requirements.txt if you want to use it? As far as I can see from the GH Actions log, this here

image

indicates that it's just not installed. Am I missing something?

bernt-matthias commented 1 year ago

Maybe this is a stupid question

There is no such thing as a stupid question :)

halirutan commented 1 year ago

The requirements.txt doesn't seem to work yet, and I had to try it locally with the Python version GH action uses. The package data-science-stubs is nowhere to be found, and I replaced it with data-science-types. Also, flake8 is unavailable in version 6.0.0 for this Python version. What worked is

black==23.3.0
data-science-types
flake8
isort==5.12.0
mypy==1.2.0
pip==22.0.4
pytest==7.1.1
types_setuptools
types-tensorflow
typing-extensions

-e .

However, running mypy results in 93 errors. So unless @mai00fti is willing to tidy up the code and make necessary changes, I recommend skipping the mypy test.

bernt-matthias commented 1 year ago

Hi @mai00fti can you check the CLI tests. Learning seems to be to fast (compared to the runtime expectations that you mentioned), ie all three calls need 2:30min.

For prediction I see two warnings: 

WARNING:tensorflow:No training configuration found in save file, so the model was *not* compiled. Compile it manually.
WARNING  No training configuration found in save file, so the model was *not* compiled. Compile it manually.

Can they be ignored?

Also convert tells me that there is nothing to convert...

INFO     Convert all data files in /home/runner/work/deepFPlearn/deepFPlearn/example/data
INFO     Found 0 files to convert