Skip to content
This repository was archived by the owner on Oct 6, 2021. It is now read-only.

KeyError fix on tram launch; ML Service class tidy-up #61

Closed
wants to merge 11 commits into from
Closed

KeyError fix on tram launch; ML Service class tidy-up #61

wants to merge 11 commits into from

Conversation

jecarr
Copy link

@jecarr jecarr commented Aug 2, 2020

Closes #77
Closes #67
Closes #63
Closes #60
Closes #59
Closes #71 -> I haven't split this PR into multiple PRs because of overlapping fixes and PRs haven't been merged in a while

A quick check to see if a description is provided in an attack or not. Plus a little documentation on multiple python interpreters in README.

Notes:

  1. Unsure if there is a standard in the codebase for strings (i.e. double or single quotes)
  2. Unsure how you want to approach missing data in a retrieved object (i.e. did you want to approach attacks with no description differently?)
  3. Unsure if I need to formally sign Developer's Certificate of Origin

Edit - I also noticed my target branch is master as opposed to develop (as requested in the Submission guidelines in above link) but at the time of writing, there is no develop branch

Edit - Updated PR to include fixes in ML service class and general tidy up. These include:

  • Prevention of invalid URL submissions added to queue
  • Missing 'awaits'
  • Exception handling in build_pickle_file()
  • Fixes for positional-argument errors
  • Removal of deprecated @asyncio.coroutine (since Python 3.8+ - test for Python 3.7 that async def is sufficient)
  • Safer file opening (added with statements)
  • Prevention of IndexError (with random.choices() call on empty lists)
  • Matches db construction to match schema.sql
  • Fixes attack retrieval from db to not break if attack is never retrieved
  • Adds word wrap as really long lines disrupt an edit-page layout

Whilst updating this PR, I've been checking similarities with others:

This initially was to fix queue issues but I see #52 exists - I believe this PR compliments that one and there is no overlap (@srbennett29, let me know if I'm wrong?)

This might also have overlap with #50 as I tried to spot PEP compliance and improve logging too - I'm happy to solve any merge conflicts if this gets merged later.


Edit - I'm not updating this PR anymore as the number of fixes have grown. If you are considering this PR (thanks!), I'd recommend creating a new one from a fork I have also been updating - here - which includes all the fixes this PR has. This is recommended to reduce conflicts.

jecarr and others added 2 commits August 2, 2020 19:57
Added caveat in Installation for anyone with multiple python interpreters
jecarr added a commit to arachne-threat-intel/thread that referenced this pull request Aug 30, 2020
@jecarr jecarr mentioned this pull request Apr 29, 2021
@jecarr jecarr changed the title KeyError fix on tram launch KeyError fix on tram launch; ML Service class tidy-up Apr 29, 2021
This was referenced Jun 2, 2021
@MarkDavidson
Copy link
Contributor

Hello @jecarr and thank you for the substantive PR. Please accept my apologies that it has taken so long to respond. TRAM has been re-architected and moved to https://github.com/center-for-threat-informed-defense/tram, which unfortunately means that very little of this PR aligns to the new code base.

From my review, it looks like most or all of the changes in this PR resolve issues with Dockerization, starting TRAM, and running TRAM. Those issues have all been addressed in the new TRAM. You can find instructions to download and run the docker container in the README.md file here https://github.com/center-for-threat-informed-defense/tram#installation.

Hopefully the new TRAM is a benefit to you. I am happy to discuss further if you would like. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants