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

feat(schematic): added synapse cache purging fds-1445 #2660

Merged

Conversation

andrewelamb
Copy link
Contributor

@andrewelamb andrewelamb commented May 2, 2024

  • fixes FDS-1445
  • Adds synapse cache purging
  • Adds deault_config.yaml for globals
  • deault_config.yaml can be overridden by config.yaml file for settable globals
  • Synapse Cache path is setable (set to /var/tmp/synapse)
  • Synapse Cache purging is setable (set to true)
  • Updated readme

@andrewelamb andrewelamb marked this pull request as ready for review May 2, 2024 18:36
@andrewelamb
Copy link
Contributor Author

@jesusaurus This is the new Schematic API we are hoping to deploy soon, that would replace the old one.

I undertsand that it was suggested by you in this PR, that the synapse cache be moved to "/var/tmp/synapse". Is that still valid?

Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

Have you tried executing one of the endpoints and see if everything is working as expected? I tried executing: http://localhost:7443/api/v1/ui/#/Storage/get_manifest_csv,
and I got an error:

{
  "detail": "local variable 'byte_size' referenced before assignment",
  "status": 500,
  "title": "Internal error"
}

But on schematic side, I tried:

store = SynapseStorage(
        access_token="<my token>", synapse_cache_path="/var/tmp/synapse"
)

and it worked...
Do you have similar errors?

size_letter = size_string[-1]
size = float(size_string[:-1])
multiple = 1024 ** size_dict[size_letter]
byte_size: int = ceil(size * multiple)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an int, thats what the ceil function does.

@ConsoleCatzirl
Copy link
Member

@andrewelamb yeah, I still have a mild preference for /var/tmp/synapse. It's a standard location for linux services to store a persistent cache, and it preemptively moves the cache to a location that a service user can own, potentially making a transition to running as a service user a little simpler.

@andrewelamb
Copy link
Contributor Author

Have you tried executing one of the endpoints and see if everything is working as expected? I tried executing: http://localhost:7443/api/v1/ui/#/Storage/get_manifest_csv, and I got an error:

{
  "detail": "local variable 'byte_size' referenced before assignment",
  "status": 500,
  "title": "Internal error"
}

But on schematic side, I tried:

store = SynapseStorage(
        access_token="<my token>", synapse_cache_path="/var/tmp/synapse"
)

and it worked... Do you have similar errors?

I can't replicate this error

@linglp
Copy link
Contributor

linglp commented May 8, 2024

Also, I think it worths noting some other changes that were made in the PR:

  • when running schematic api locally, purge_synapse_cache function will now purge the cache of users. In the current schematic code base, I think it only purge caches on AWS. When users run schematic APIs locally, we don't touch their cache. I am not sure if this is a "desired" behavior since "users" will most likely to be developers and probably aware of what's going on.
  • And also worth noting that we now clear cache before calling manifest download endpoint.

Copy link

sonarqubecloud bot commented May 9, 2024

@andrewelamb andrewelamb requested a review from linglp May 9, 2024 14:43
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

Thanks. I think it looks good to me.

@andrewelamb andrewelamb merged commit 0daed1b into Sage-Bionetworks:main May 9, 2024
10 checks passed
@andrewelamb andrewelamb deleted the feature-cache-purging-fds-1445 branch May 9, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants