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

minimal changes to allow changing database path #19

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

ilia-nikiforov-umn
Copy link
Contributor

@ilia-nikiforov-umn ilia-nikiforov-umn commented Jul 30, 2024

This minimal change allows changing the path of the local database. This is necessary to allow the Docker image to be exported to an operational Singularity container that is able to use the local database (since singularity uses the host filesystem and therefore cannot write to /pipeline/db/). So far, kimitems install and pipeline-run-pair is confirmed working using Singularity. All other necessary paths should be able to be set using a custom file specified with the PIPELINE_ENVIRONMENT_FILE environment variable passed into the container.

@ilia-nikiforov-umn ilia-nikiforov-umn self-assigned this Jul 30, 2024
@ilia-nikiforov-umn ilia-nikiforov-umn marked this pull request as draft July 31, 2024 00:10
@ilia-nikiforov-umn ilia-nikiforov-umn marked this pull request as ready for review July 31, 2024 16:34
@ilia-nikiforov-umn
Copy link
Contributor Author

@dskarls these minimal changes allow for everything the KDP does (at least in the process of pipeline-run-pair, including local dependencies) to be contained within a custom specified directory, which allows it to be run using singularity on a cluster. Also, it seems that the KIM_API_USER_COLLECTION variable wasn't doing anything, I grepped for it and it wasn't referenced anywhere.

I believe this doesn't break anything in the main branch, do you see this causing any problems?

Copy link
Contributor

@dskarls dskarls left a comment

Choose a reason for hiding this comment

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

I see no problems whatsoever with the db path customization. As long as the KIM API is handling models properly, you can merge this.

USE_FULL_ITEM_NAMES_IN_REPO=True

# parent directory where things are run by default
WORKER_RUNNING_PATH=/tmp

# We install MD/MO/SM to the KIM API user collection
KIM_API_USER_COLLECTION=/home/openkim/.kim-api/
Copy link
Contributor

@dskarls dskarls Aug 4, 2024

Choose a reason for hiding this comment

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

KIM_API_USER_COLLECTION was a special envvar that the KIM API used to inspect when figuring out where to install or load models but it may no longer use it (and it was also only one way to specify that information). As long as you've confirmed that models are installed where you need them to be and are accessible when doing things like pipeline-run-pair, this should be fine.

Copy link
Contributor Author

@ilia-nikiforov-umn ilia-nikiforov-umn Aug 7, 2024

Choose a reason for hiding this comment

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

@dskarls

Confirmed that the current version of the API doesn't know anything about this variable (both commands return nothing)

ilia@ilia-xps:~/kim-api-2.3.0$ grep KIM_API_USER_COLLECTION README.md 
ilia@ilia-xps:~/kim-api-2.3.0$ find -type f | xargs grep KIM_API_USER_COLLECTION 

@dskarls dskarls added the enhancement New feature or request label Aug 4, 2024
@ilia-nikiforov-umn ilia-nikiforov-umn merged commit d746f62 into main Aug 7, 2024
6 checks passed
@ilia-nikiforov-umn ilia-nikiforov-umn deleted the singularity-minimal-changes branch August 7, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants