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

Add the UNAVAILABLE certificate. #115

Merged

Conversation

itrajanovska
Copy link
Collaborator

No description provided.

@BigDatalex
Copy link
Collaborator

Hey,
I think it would be great to have some description of all PRs, even if it's just a small one. So, in this case, I think it is kinda important to mention that this change affects the ecosia-exporter repo (see here), because the poetry environment of the exporter references this branch.

We have talked about it in the morning, but honestly, I don't feel comfortable with approving this. As an example, we have also added other (regular) sustainability labels and these wouldn't be part of this branch. In other words, what speaks against merging the main branch into this one?

Maybe @se-jaeger can have a look at this and say if there might be any implications.

@itrajanovska
Copy link
Collaborator Author

Hi,

I didn't add any documentation as I didn't know that that was a requirement for every MR. But for this small of a change I thought making this MR was a step enough to make everyone aware that we need to update the special labels file - and as added in the json change itself there is a description in German already of why that label is needed.

"description": "Dieses Label wird für Produkte verwendet, die kein Nachhaltigkeits-Label haben, aber ein anderes, aus einem ML-Modell abgeleitetes Label tragen."

I think the new labels should be added in this (test-database-with-new-poetry) branch as well, as if I'm not mistaken, they won't pass the main::to_df method, as there are constraints in the Product class, specifically for the sustainability_labels in this case and which values are allowed to it.
And, after checking the labels - core/*.json files, there wasn't any change by comparing them with the main branch.

Also, regarding why are we using that branch, I haven't seen any comment left on that anywhere, and I don't know if it's safe to merge the main branch in it, but when trying to switch to the main branch in the dependencies section once, I got the following message:

Because database (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=database depends on core (0.2.4) @ file:///Users/ivana/DataSpellProjects/entity-matcher/.venv/src/green-db/core
 and entity-matcher depends on core (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=core, database is forbidden.
So, because entity-matcher depends on database (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=database, version solving failed.

... So, that's why I assumed there are some dependency issues, and that's why I continued adding my changes on that branch.

@se-jaeger
Copy link
Contributor

I think it would be much nicer to fix this one #94 and use this repos' main branch for the ecosia-exporter as dependency.
Then we don't need this branch anymore.

@itrajanovska
Copy link
Collaborator Author

itrajanovska commented Jan 18, 2023

I think it would be much nicer to fix this one #94 and use this repos' main branch for the ecosia-exporter as dependency. Then we don't need this branch anymore.

Thanks for the prompt reply @se-jaeger !
For the sake of shipping the EM model with next weeks data extraction I would still add this now as a quick fix.
Even if it's not for the EM model only, the Ecosia export will fail as well because of the main::to_df issue that I mentioned above.
I'd be happy to get back to this once I have more time and solve it properly. However for now - given the priorities and the fact that this branch existed for a longer period due to an unrelated long-running issue, I think this is a good quick fix.

@itrajanovska itrajanovska merged commit a2affc8 into test-database-with-new-poetry Jan 18, 2023
@itrajanovska itrajanovska deleted the add_unavailable_certificate branch January 18, 2023 20:47
@itrajanovska
Copy link
Collaborator Author

Thanks for the feedback in the DM Alex,

I also misunderstood your point in the first place.
Now I just merged main into this branch accordingly and no further changes will be done on this branch unless we have them on main.

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.

None yet

3 participants