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

Update INSTALL docs #811

Merged
merged 3 commits into from May 27, 2021
Merged

Update INSTALL docs #811

merged 3 commits into from May 27, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented May 24, 2021

  • Fix dead link in CONTRIBUTING
  • Added some instructions for testing other backends with docker

@ml-evs ml-evs added docs Issues pertaining to documentation. priority/medium Issue or PR with a consensus of medium priority labels May 24, 2021
INSTALL.md Outdated
&& OPTIMADE_DATABASE_BACKEND="elastic" py.test

# Run tests against a real containerized MongoDB
docker container stop mongo_test; docker container rm mongo_test; \
Copy link
Contributor

@JPBergsma JPBergsma May 25, 2021

Choose a reason for hiding this comment

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

1 Perhaps you could clarify a bit what these steps do?
It seems a bit strange that you try to stop the container before you start it.

2 I had to run the commands with sudo privileges.

3 For me the complete test hangs on "test_structures.py". If I do the complete test outside docker the same error occurs, so I do not think it depends on your instructions. Strangely enough, running test_structures.py individually does pass all the tests. I was thus not able to complete the docker instructions.

With the elastic search the test did complete normally although there seemed to be fewer tests than for Mongodb.

Copy link
Member Author

Choose a reason for hiding this comment

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

1 Perhaps you could clarify a bit what these steps do?
It seems a bit strange that you try to stop the container before you start it.

This is just to provide a single repeatable command, the docker image needs to be destroyed either before/after testing as otherwise it will contain the test data. Perhaps it makes more sense to have it at the end.

2 I had to run the commands with sudo privileges.

This is quite system-dependent and I would prefer to not ask people to use sudo by default (though we can link to the docker documentation); on Arch, installing docker automatically adds the user to the docker group so you can run without sudo.

3 For me the complete test hangs on "test_structures.py". If I do the complete test outside docker the same error occurs, so I do not think it depends on your instructions. Strangely enough, running test_structures.py individually does pass all the tests. I was thus not able to complete the docker instructions.

Strange... could you post the full output (maybe with py.test -vvv too?)

With the elastic search the test did complete normally although there seemed to be fewer tests than for Mongodb.

There is a set of tests that are run for both backends, and some that can only be run on one or the other, so this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. When there are line breaks, the terminal thinks you have pressed enter and executes the statement.
    https://askubuntu.com/questions/377259/stop-terminal-auto-executing-when-pasting-a-command
    This is usually not what you want. As you still want to be able to modify the statement.

2 You are right It is better not to use sudo by default.

3 It seems to hang in models/test_structures.py::test_bad_structures it also hangs when I run only test_structures.py (There are two files named test_structures hence the confusion.) I was still running it using my code branch. So perhaps my changes in mongo.py broke something. I'll look into this tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

1. When there are line breaks, the terminal thinks you have pressed enter and executes the statement.
   https://askubuntu.com/questions/377259/stop-terminal-auto-executing-when-pasting-a-command
   This is usually not what you want. As you still want to be able to modify the statement.

The continuation characters (/) should prevent this (or at least do for me under sh, bash, zsh and fsh...). I did spot a missing / in the elastic instructions.

2 You are right It is better not to use sudo by default.

3 It seems to hang in models/test_structures.py::test_bad_structures it also hangs when I run only test_structures.py (There are two files named test_structures hence the confusion.) I was still running it using my code branch. So perhaps my changes in mongo.py broke something. I'll look into this tomorrow.

These lines basically follow what we do in the CI, so they should work everywhere on the latest commit.

I'll expand a bit on the comments to make clear what we are doing, and perhaps will reorganize the calls so that the images are deleted after the tests, not before the next tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

The continuation characters (/) should prevent this (or at least do for me under sh, bash, zsh and fsh...). I did spot a missing / in the elastic instructions.

It seems git hub places an extra end of line character at the end. I still have to add an extra (\) at the end to prevent automatic execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you looking at the rendered one here https://github.com/Materials-Consortia/optimade-python-tools/blob/78685f5d6d859e6ed635174689b03bfd7a5807bd/INSTALL.md? I did add an extra / for elastic in my last commits, as I said above

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I looked at the rendered version.

Copy link
Contributor

Choose a reason for hiding this comment

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

3 It seems to hang in models/test_structures.py::test_bad_structures it also hangs when I run only test_structures.py (There are two files named test_structures hence the confusion.) I was still running it using my code branch. So perhaps my changes in mongo.py broke something. I'll look into this tomorrow.

It turned out to be a problem with the conda environment. Removing the old environment and creating a new one solved the problem. I still have no idea why the original environment stopped functioning.

JPBergsma
JPBergsma previously approved these changes May 25, 2021
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

I think it is possible to automatically set up link checking for MD documents on github.
See for example https://github.com/gaurav-nelson/github-action-markdown-link-check
(I did not know where I could see whether you already use such a solution so you can ignore this remark if you have already set up.)

Perhaps you could explain the individual commands briefly. Having it as 1 statement made it more difficult to understand and also made it harder to paste it in the terminal.

@ml-evs
Copy link
Member Author

ml-evs commented May 25, 2021

I think it is possible to automatically set up link checking for MD documents on github.
See for example https://github.com/gaurav-nelson/github-action-markdown-link-check
(I did not know where I could see whether you already use such a solution so you can ignore this remark if you have already set up.)

We could do this, but it is a bit self-referential as the self-links are not "up" until the documentation has been built and pushed to https://optimade.org/optimade-python-tools.

Perhaps you could explain the individual commands briefly. Having it as 1 statement made it more difficult to understand and also made it harder to paste it in the terminal.

One statement should make it easier to paste (and then re-use from the shell history). In the rendered docs there is even a clipboard option for each code snippet: https://www.optimade.org/optimade-python-tools/INSTALL/ (which may mean that this is worth separating into two code snippets)

@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #811 (78685f5) into master (3d4e19b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #811   +/-   ##
=======================================
  Coverage   92.68%   92.68%           
=======================================
  Files          68       68           
  Lines        3677     3677           
=======================================
  Hits         3408     3408           
  Misses        269      269           
Flag Coverage Δ
project 92.68% <ø> (ø)
validator 92.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d4e19b...78685f5. Read the comment docs.

@ml-evs
Copy link
Member Author

ml-evs commented May 27, 2021

Thanks for the thorough review @JPBergsma!

@ml-evs ml-evs merged commit 0f11919 into master May 27, 2021
@ml-evs ml-evs deleted the ml-evs/update_install_docs branch May 27, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues pertaining to documentation. priority/medium Issue or PR with a consensus of medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants