-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/ Expertise selection: add experimental new UI #1949
base: master
Are you sure you want to change the base?
Conversation
It seems you are overwriting the default expertise implementation. I suggest to implement a new component for your implementation. |
I agree with Melisa here. When we discussed this last I thought I made clear that you needed to create a new component with a different (descriptive) name that would be enabled by changing the webfield configuration for the venue group. |
components/ExpertiseSelector.js
Outdated
}, | ||
null, | ||
{ accessToken, includeVersion: true } | ||
const keyphraseResponse = await fetch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the included api
lib to make API requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of the api
lib seems to be for relative requests on the openreview-api. It prefixes the path
with a /
if it doesn't begin with it already. Additionally, the baseUrl
used is the API_URL
. I'm looking at the implementation here. Please let me know if I should modify the lib to allow making absolute requests to other domains.
components/ExpertiseSelector.js
Outdated
null, | ||
{ accessToken, includeVersion: true } | ||
const keyphraseResponse = await fetch( | ||
'https://retrievalapp-zso5o2q47q-uc.a.run.app/get_recommendations/mccallum' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All endpoint URLs should be configurable usings env
vars. Also why is this pointing just to recommendations for mccallum? Shouldn't this component pass the ID of the currently logged in user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our backend model currently only has the data for a few limited users and might not support all users with an openreview account at present. @MSheshera might be able to share more information.
components/ExpertiseSelector.js
Outdated
) | ||
}, [keyphrases]) | ||
|
||
function hashString(s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use a generic hash function from a library or at least extract these functions into lib/utils.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Moved into lib/utils.js
. lib/forum-utils has a getInvitationColors
which I took inspiration from but it seems limited to 15 bg colors. For the controllable expertise selector UI we currently limit to upto 20 keyphrases (each requiring a unique color). Let me know your thoughts!
components/ExpertiseSelector.js
Outdated
</div> | ||
<div className="mb-3 ml-3"> | ||
Potential assignments from papers accepted in machine learning conferences from | ||
2018 to 2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text and these dates shouldn't be hardcoded, it could come from config vars or another source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This div
might get removed in a future iteration. If not where do you suggest I get this? I couldn't find a specific config
var in the openreview-web
codebase, however I'm aware of config
within the v1/v2 APIs being present.
@@ -21,6 +22,7 @@ export default function ExpertiseConsole({ appContext }) { | |||
} = useContext(WebFieldContext) | |||
const [shouldReload, reload] = useReducer((p) => !p, true) | |||
const router = useRouter() | |||
const useControllableExpertiseSelector = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this value must be a parameter of the ExpertiseConsole and it should be false by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is missing, could you please add it so we can merge this soon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Melisa. Refactored it to be a parameter which can be passed from here.
Please let me know if you'd like this to be a part of the webComponentProps
instead, although I'm not sure how componentObj should be modified only for ExpertiseConsole while still keeping the code generalized.
} | ||
async function fetchRecommendations() { | ||
const baseUrl = | ||
'https://retrievalapp-zso5o2q47q-uc.a.run.app/get_recommendations/mccallum' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a parameter of the component?
does it work for all the venues or only specific ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can have it be the parameter of the component or if there's a constants
file I can add the baseUrl
to that and get the username using one of the existing methods.
The user expertise is agnostic of the venue. This should work for all venues. cc: @MSheshera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this will not work for all venues. This backend caches reviewer data and reviewers will be specific to a venue. Venues may also choose to show "Potential Paper Assignments" using a domain-specific collection e.g. while NLP/ML venues may use papers from those veneues (as we are now) Computer Vision venues may use a different collection of papers.
This can be changed in future designs but for a first run, this is easiest to set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you handle authentication here? is this link public for any user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosmondra indicates that this server must support cross domain so it can be called from openreview.net.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you handle authentication here? is this link public for any user?
We havent added any authentication to the API call yet. This is one of our immediate todos - would you prefer for us to complete this before this PR is accepted?
this server must support cross domain so it can be called from openreview.net.
What do you mean? It should be possible to call the server from openreview.net.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authentication must be supported if we are going to ask the OpenReview users to send and receive sensitive data from your servers.
In order to call to a server with a different domain: from openreview.net to your sheshera.server.com, your server must accept calls from a different domain for security reasons. More info here: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
@MSheshera I don't find the API call that stores the keyphrases in your server, can you point me at the code please? |
The code is here: https://github.com/MSheshera/lace_deployment/ And some preliminary documentation for the API: https://retrievalapp-zso5o2q47q-uc.a.run.app/docs# hmm, @RonLek I presume there is some part of your code where you make an API call to add keyphrases if a reviewer adds it. Can you point Melisa to that as well? But some more context, the initial set of keyphrases for a reviewers profile will be computed offline and added to the API by a separate cron job running on Unity. Implementing this is a todo as of now, but for now the backend has a hardcoded set of reviewers and their profiles to help debug the front end. |
This happens here and is triggered if The backend infers when it's an edit (add/delete) operation ( Docstring:
|
I don't think I understand, that code is doing a fetch operation and not a post. Where do you send the data from the UI to your server to store it in your database? |
I see. The same GET endpoint is used to update the session variables. I agree this should be managed by a separate POST endpoint on the backend (cc: @MSheshera). Also, fyi currently there is no database on the backend. All user variables are session variables (and are lost if the server is stopped). We're currently looking to use Redis on GCP for this.
The |
Thanks for the details but if we are going to send sensitive data to your server this must be a POST operation and there should be authentication.
What does it mean? if the server is down, the expertise selected by the users is lost? |
I can not access to https://github.com/MSheshera/lace_deployment, could you send me the invitation again? |
Yes of course, I'll make the change on the backend.
This is our current implementation. We will update the backend to use Redis to retain all the reviewer data and the changes induced by their interactions across sessions. However, I'd like to understand this: The current backend was mainly built to support the development of the front end and several of the changes being discussed above are being worked on at the moment, do you prefer for us to make these changes before this PR is accepted? @melisabok: I also invited you to the backend repo. https://github.com/MSheshera/lace_deployment/invitations |
The UI must work with a server that support authentication, POST operation to send data from the UI to the server and persist the user changes in a data store that can be recovered if the server is restarted. |
Got it! We'll prioritize implementing those. |
eb524ec
to
453d7e7
Compare
Feat: