Skip to content

Standard integration tests, PEP8, docker adjustments #23

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 22 commits into from
Jul 12, 2022
Merged

Standard integration tests, PEP8, docker adjustments #23

merged 22 commits into from
Jul 12, 2022

Conversation

@CBroz1
Copy link
Contributor

@CBroz1 CBroz1 commented May 4, 2022 •
edited
Loading

  • PEP8
  • Draft integration tests from element-calcium-imaging
  • Update 05-explore Jupyter notebook #20
  • adjusted docker to resolve
    1. permissions issues declaring schemas on the db
    2. issues when connecting to paired docker container bad handshake
  • Remaining issue: Built result has python 3.10 (thus encountering collections.Mapping error), even when installing from datajoint/djbase:py3.7-debian-8eb1715

@CBroz1 CBroz1 added this to the NIH U24 Year 3 milestone May 4, 2022
@CBroz1 CBroz1 self-assigned this May 4, 2022
@CBroz1 CBroz1 changed the title WIP Standard integration tests, PEP8, docker adjustments May 5, 2022
@CBroz1 CBroz1 added the CI/CD label May 6, 2022
@CBroz1 CBroz1 requested a review from kabilar May 6, 2022 21:30
@CBroz1 CBroz1 marked this pull request as ready for review May 6, 2022 21:30
@CBroz1 CBroz1 assigned kabilar and unassigned CBroz1 May 7, 2022
This was linked to issues May 11, 2022
Create integration tests with pytest #8
Closed
Update 05-explore Jupyter notebook #20
Closed
@CBroz1
Revised docker from @iamamutt
0cfdcf3 
tests/test_ingest.py::test_ingest_subjects PASSED
tests/test_ingest.py::test_ingest_sessions PASSED
tests/test_ingest.py::test_paramset_insert ERROR
tests/test_pipeline_generation.py::test_generate_pipeline PASSED
tests/test_populate.py::test_recording_info_populate PASSED
tests/test_populate.py::test_processing_populate ERROR

ERRORs because AttributeError: module 'collections' has no attribute 'Mapping'
@CBroz1
Copy link
Contributor Author

CBroz1 commented May 13, 2022

@kabilar - After a lot of debugging, I looped back around to the same python 3.10 problem, even when installing from a 3.9 image. I think CaImAn's installation process upgrades python. Wait for datajoint-python PR 1002

With @iamamutt's help, the docker file more user-friendly to pick specific forks. Maybe before Q3, we'll be able to standardize across the elements dockers. Get all CI/CD up-to snuff before major dev on the Y3 additions?

kabilar
kabilar reviewed May 13, 2022
View reviewed changes
kabilar
kabilar reviewed May 13, 2022
View reviewed changes
kabilar
kabilar reviewed May 13, 2022
View reviewed changes
@CBroz1 CBroz1 assigned CBroz1 and unassigned kabilar May 17, 2022
@CBroz1
Copy link
Contributor Author

CBroz1 commented May 23, 2022

If I run the docker compose in detached mode, the log shows passing all tests. If I run without this flag, I see bad connection errors. @kabilar - what do you want to do?

docker-compose -f ./docker/docker-compose-test.yaml up --build --force-recreate --detached # success
docker-compose -f ./docker/docker-compose-test.yaml up --build --force-recreate # errors as ...
pymysql.err.OperationalError: (2003, "Can't connect to MySQL server on 'db' ([Errno 111] Connection refused)")

@CBroz1 CBroz1 assigned kabilar and unassigned CBroz1 May 23, 2022
@kabilar
Copy link
Collaborator

kabilar commented May 24, 2022

If I run the docker compose in detached mode, the log shows passing all tests. If I run without this flag, I see bad connection errors. @kabilar - what do you want to do?

Thanks for looking into this @CBroz1! Let's just run in the --detached mode for the pytests. I will review this pull request.

@CBroz1 CBroz1 requested a review from kabilar July 8, 2022 17:26
@CBroz1 CBroz1 linked an issue Jul 8, 2022 that may be closed by this pull request
Update 05-explore Jupyter notebook #20
Closed
kabilar
kabilar reviewed Jul 11, 2022
View reviewed changes
kabilar
kabilar reviewed Jul 11, 2022
View reviewed changes
kabilar
kabilar reviewed Jul 11, 2022
View reviewed changes
@kabilar kabilar mentioned this pull request Jul 11, 2022
Open
@kabilar
Copy link
Collaborator

kabilar commented Jul 11, 2022

Hi @CBroz1, are the items from the first comment above now addressed?

  • Draft integration tests from element-calcium-imaging, needs testing

  • pytests succeeded locally, encountered issues with docker containers

    1. permissions issues declaring schemas on the db
    2. issues when connecting to paired docker containter bad handshake, which @iamamutt recommends setting the following config to fix "database.use_tls": false
    3. Built result has python 3.10 (thus encountering collections.Mapping error), even when installing from datajoint/djbase:py3.7-debian-8eb1715

@kabilar
Copy link
Collaborator

kabilar commented Jul 11, 2022 •
edited
Loading

I made adjustments to this notebook to provide the plots below. Maybe not the most elegant solution in each case? np arrays needed some reworking. Maybe that's an issue with how we're storing them?

Thanks! I am not sure why this would be the case, but I don't see an issue with reshaping the arrays. Perhaps add any details to Issue #25 and we can address in the future.

kabilar
kabilar reviewed Jul 11, 2022
View reviewed changes
kabilar
kabilar requested changes Jul 11, 2022
View reviewed changes
Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @CBroz1! A few minor comments above.

@kabilar kabilar assigned CBroz1 and unassigned kabilar Jul 11, 2022
dimitri-yatsenko
dimitri-yatsenko requested changes Jul 11, 2022
View reviewed changes
dimitri-yatsenko
dimitri-yatsenko reviewed Jul 11, 2022
View reviewed changes
dimitri-yatsenko
dimitri-yatsenko reviewed Jul 11, 2022
View reviewed changes
dimitri-yatsenko
dimitri-yatsenko reviewed Jul 11, 2022
View reviewed changes
CBroz1 and others added 4 commits July 12, 2022 09:05
@CBroz1 @kabilar @dimitri-yatsenko
Apply suggestions from code review
d7165ac 
Co-authored-by: Kabilar Gunalan <[email protected]>
Co-authored-by: Dimitri Yatsenko <[email protected]>
@CBroz1
WIP. See details. Jupysync, verbose context, abc.Seq, etc.
8435df2 
notebooks: Run Jupytext sync
tests/__init__ : set conditional verbose context in constants, remove numpy
workflow_mini/paths: list->abc.Sequence
docker/docker-compose-test: uncomment volumes
@CBroz1
WIP: verbosity if to oneliner
b1e45a2
@CBroz1 @kabilar
Version bump
68b04ed 
Co-authored-by: Kabilar Gunalan <[email protected]>
@CBroz1 CBroz1 assigned kabilar and unassigned CBroz1 Jul 12, 2022
kabilar
kabilar reviewed Jul 12, 2022
View reviewed changes
@CBroz1 CBroz1 mentioned this pull request Jul 12, 2022
Closed
@CBroz1
Copy link
Contributor Author

CBroz1 commented Jul 12, 2022

Thanks for your review @kabilar - I've opened the issue we discussed (#26) and updated notebooks to revert the black formatting corruption

kabilar
kabilar approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @CBroz1! Great work.

@kabilar kabilar merged commit 9975d8a into datajoint:main Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dimitri-yatsenko dimitri-yatsenko dimitri-yatsenko requested changes

@kabilar kabilar kabilar approved these changes

+2 more reviewers

@iamamutt iamamutt iamamutt left review comments

@jverswijver jverswijver jverswijver left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kabilar kabilar

Labels

CI/CD

Projects

None yet

Milestone

2022Q3

Development

Successfully merging this pull request may close these issues.

Update 05-explore Jupyter notebook Create integration tests with pytest

5 participants

@CBroz1 @kabilar @dimitri-yatsenko @iamamutt @jverswijver