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

Modifying the local Catalog functionality #1195

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

Malikbadmus
Copy link
Contributor

@Malikbadmus Malikbadmus commented Jul 10, 2024

Thank you for taking your time to contribute to Ersilia, just a few checks before we proceed

  • Have you followed the guidelines in our Contribution Guide
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Description

Utilizing information.json instead of the AirTable database to populate the local model card.

Changes to be made

  • Modified the dir path in the two (2) LocalCard class method.
  • Made use of lru_cache to cache the results.

Status

ersilia catalog --local --more command now takes between 0.5s-0.8s, representing an almost 97% improvement in performance.

Related to #1072

@Malikbadmus Malikbadmus changed the title Catalog Modifying the local Catalog functionality Jul 10, 2024
@@ -30,6 +30,7 @@
LOGGING_FILE = "console.log"
CURRENT_LOGGING_FILE = "current.log"
CARD_FILE = "card.json"
LOCAL_CARD_FILE = "information.json"
Copy link
Member

Choose a reason for hiding this comment

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

We already have a file called INFORMATION_FILE, do we need to create another one?

@@ -45,7 +46,7 @@
except:
Hdf5Explorer = None

from ...default import CARD_FILE, METADATA_JSON_FILE, SERVICE_CLASS_FILE
from ...default import EOS, LOCAL_CARD_FILE, METADATA_JSON_FILE, SERVICE_CLASS_FILE
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand where 'LOCAL_CARD_FILE' is being written to for your code to be able to read it. If it is the same information file as i mentioned above, in that case, you can reuse that same identifier instead of creating a new one.

@Malikbadmus
Copy link
Contributor Author

@DhanshreeA , you are right, there's no need to create a new file, when we have an existing file that serves the same function.

I've modified the PR to reflect this.

ersilia/hub/content/card.py Outdated Show resolved Hide resolved
ersilia/hub/content/card.py Outdated Show resolved Hide resolved
@DhanshreeA
Copy link
Member

Looks pretty good @Malikbadmus , thanks for implementing all the suggestions. Merging this now.

@DhanshreeA DhanshreeA merged commit 3b89bf0 into ersilia-os:master Jul 15, 2024
16 checks passed
@Malikbadmus Malikbadmus deleted the catalog branch July 17, 2024 17:40
@Malikbadmus Malikbadmus restored the catalog branch August 10, 2024 20:18
@Malikbadmus Malikbadmus deleted the catalog branch August 10, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants