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

Enhance history with filterable content #1562

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

Anindyadeep
Copy link
Contributor

This PR adds datetime (standard ISO format and some more parameters) to the history so that it could be easily searchable / filterable. Additionally this PR enhances the inspect_history function by adding the datetime timestamp into it. Which shows like this:

Screenshot 2024-09-30 at 12 06 35 AM

Fixes issue: #1073

Additionally enhance inspect_history function with datetime
@okhat
Copy link
Collaborator

okhat commented Sep 30, 2024

This is great, thank you @Anindyadeep ! But if it’s not too much trouble could you keep the format / style of the code — especially not in the same PR that changes functionality.

@Anindyadeep
Copy link
Contributor Author

This is great, thank you @Anindyadeep ! But if it’s not too much trouble could you keep the format / style of the code — especially not in the same PR that changes functionality.

Hey, so I actually installed pre-commit, I am not sure, that formatting might be caused by that.

@okhat
Copy link
Collaborator

okhat commented Oct 2, 2024

@Anindyadeep Maybe the pre-commit you have is outdated? It just changed the file a bit too much IMO, it will hurt other WIP merges.

@okhat
Copy link
Collaborator

okhat commented Oct 7, 2024

@Anindyadeep I would love to merge this and I think the real change is just this one line right?

        entry = dict(
            timestamp=datetime.now().isoformat(),
            uuid=str(uuid.uuid4()),
            model=self.model,
            model_type=self.model_type,
        )

Could you make the PR change just this one line if so (plus any required imports and any other changes I missed). If too busy, I can make that change and credit you in a different PR.

@Anindyadeep
Copy link
Contributor Author

@Anindyadeep I would love to merge this and I think the real change is just this one line right?

        entry = dict(
            timestamp=datetime.now().isoformat(),
            uuid=str(uuid.uuid4()),
            model=self.model,
            model_type=self.model_type,
        )

Could you make the PR change just this one line if so (plus any required imports and any other changes I missed). If too busy, I can make that change and credit you in a different PR.

Ahh sorry, I missed to add the changes. Let me do it in some time.

@Anindyadeep
Copy link
Contributor Author

@okhat ready for merge, sorry for delaying it :)

@okhat okhat merged commit bae2ad8 into stanfordnlp:main Oct 7, 2024
4 checks passed
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.

2 participants